Add filters to Running Compactions table in monitor #4986
Conversation
|
I added 9a218d6 to show how I was injecting the test data. If anyone wants to try these changes out you could use that commit in some form. Here is what the table looks like with some sample filters applied and one invalid filter: |
DomGarguilo
changed the title
|
Marking as ready for review. The duration filter has been added and everything is working as intended as far as I can tell. If anyone wants to try out these changes, I would recommend checking out 4828fdb which was before the test data was removed. |
|
I tried running continuous ingest to generate compactions. Was able to create lots of compactions that I could see in the shell using listcompactions, however I never saw any on this page. While running this test I ran into #4998, #4999,apache/accumulo-testing#285, and apache/accumulo-testing#284 |
There was a problem hiding this comment.
These changes look really nice and work well. Styling is consistent with the rest of the Monitor pages. Filtering works well and as expected. I like the choice on having a "Toggle Filters" button. Left a few small code comments, and a few general comments below:
- Duration filter shows "invalid duration format" after invalid entry is cleared and the box is empty. Would be nice if this message did not show in this case. This is not true for the other filters. Not sure how simply this can be done in current code, can ignore if changes are too large.
- Noticed if filters are applied and then the "Toggle Filters" is turned back off, the filters are still applied to the table. It might be nicer if the filters are cleared on "Toggle Filters". This is just a style choice, so feel free to ignore if you prefer the current impl. Maybe if it is kept as it currently is "Toggle Filters" could be changed to "Show Filters".
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/ec.js
Outdated
Show resolved
Hide resolved
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/ec.js
Show resolved
Hide resolved
| default: | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| // Helper function to convert duration strings to seconds | ||
| function convertToSeconds(value, unit) { | ||
| switch (unit.toLowerCase()) { | ||
| case 's': | ||
| return value; | ||
| case 'm': | ||
| return value * 60; | ||
| case 'h': | ||
| return value * 3600; | ||
| case 'd': | ||
| return value * 86400; | ||
| default: | ||
| return value; |
There was a problem hiding this comment.
212 and 228
Should these throw an error instead? We wouldn't expect the default to occur, correct?
There was a problem hiding this comment.
Addressed in 263b617. I added console.error logs. I dont think we want things to break if the default case is reached but logging an error seems like a good idea to indicate something has gone wrong.
Good catch, I did not notice that. I can fix that pretty easily I think.
Yea the "Toggle Filters" button is a bit misleading. It just minimizes the input fields. The filters are still applied while its minimized. Maybe a different indicator other than "toggle" should be used. Also, adding a clear filters button sounds like a good idea. |
| * @param {number} durationStr duration in milliseconds | ||
| * @returns duration in seconds | ||
| */ | ||
| function parseDuration(durationStr) { |
There was a problem hiding this comment.
I tried running this again and figured out why I was not seeing any data. I had some config in my browser that was sending /rest/ec/running request to /dev/null.
Once I fixed that I could see data and noticed compaction ages were in years. Looked into this and I think its a unit mismatch. RunningCompactionInfo.java has the following line
duration = last.getCompactionAgeNanos();This function is expecting millis and maybe its getting nanos.
There was a problem hiding this comment.
Yea it looks like thats the case. I thought it was a bug with how I was creating the sample data. The bug might be present in 2.1 as well.
There was a problem hiding this comment.
Hmm. I didn't notice this. For me, the sample data seemed normal (nothing over a couple hrs)
There was a problem hiding this comment.
Should be fixed via #5007. @keith-turner @kevinrr888
There was a problem hiding this comment.
When experimenting with these changes I was using accumulo-testing cingest to generate compactions. I started 10 compactor processes locally and ran ingest for a bit. The compactions were normally really fast and would not show up reliably. After a bit of data had built up, I forced full table compactions to create longer running compactions that I could look at in the monitor.
|
I think I changed to be millis and pushed that in another commit so you
might have been looking at that.
…On Mon, Oct 21, 2024 at 4:11 PM Kevin Rathbun ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/ec.js
<#4986 (comment)>:
> + return value * 86400;
+ default:
+ return value;
+ }
+ }
+
+ // Helper function to validate duration input. Makes sure that the input is in the format of '<operator> <value> <unit>'
+ function validateDurationInput(input) {
+ return input.match(/^([<>]=?)\s*(\d+)([smhd])$/i);
+ }
+
+ /**
+ * @param {number} durationStr duration in milliseconds
+ * @returns duration in seconds
+ */
+ function parseDuration(durationStr) {
Hmm. I didn't notice this. For me, the sample data seemed normal (nothing
over a couple hrs)
—
Reply to this email directly, view it on GitHub
<#4986 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALMD2IIKOOEBW5NVEHBBF63Z4VNVFAVCNFSM6AAAAABP75WPR6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOBTGIYTIMBZHA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Fixes #4940
This PR adds regex filtering to the Running Compactions table on the External Compactions page in the monitor.
Features:
This is marked as WIP because the ticket requested age filtering and that has yet to be added.