-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add filters to Running Compactions table in monitor #4986
base: main
Are you sure you want to change the base?
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: |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/ec.js
Outdated
Show resolved
Hide resolved
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. |
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/ec.js
Show resolved
Hide resolved
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.