-
Notifications
You must be signed in to change notification settings - Fork 463
Added --json flag to ec-admin running command #5332
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
Conversation
@kevinrr888 I hope this fixes the fix the issue with the commit history :) |
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.
General comments. I suggest running mvn clean verify -Psunny
before committing future changes. This will catch a lot of potential issues with new code.
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
…min.java Co-authored-by: Kevin Rathbun <[email protected]>
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
Concerns regarding build failures have been addressed. No further comments at this time
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/util/ECAdmin.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Dave Marion <[email protected]>
This looks pretty good. Have you tested this locally? |
@dlmarion No, I did not understand how to setup the development environment but I wanted to contribute so I decided to open a pr regardless. Sorry about that😅 |
@Suvrat1629 - I would suggest looking at https://github.com/apache/fluo-uno to get a development environment set up so that you can test this locally. |
I ran this command and got done with the set up. |
@Suvrat1629 - looking at the fluo-uno readme, it looks like you should be able to set the ACCUMULO_REPO environment variable in the uno.conf file and then fluo-uno will build Accumulo from your feature branch. See https://github.com/apache/fluo-uno/blob/main/conf/uno.conf#L46. Once everything is running, Accumulo should be running from the directory specified by the ACCUMULO_HOME variable (see https://github.com/apache/fluo-uno/blob/main/conf/uno.conf#L126). You should be able to run the |
I like the idea of having a standard computer-readable output, but I'd prefer not to have CSV as an option. There are multiple different CSV standards, and they all do things slightly differently with quoting, spacing, redundant delimiters, etc. Plus, Java's String.split function is very confusing with splitting on a delimiter, making it harder to parse correctly. JSON or some other standard is fine, though. |
I think the csv output is due to how I wrote the issue ticket. I believe a json only output would be satisfactory to close the issue ticket. |
Refactored the ECAdmin command and added a test. Downside to this implementation is that it will consume more memory in the case where json output is not requested.
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.
Looks good to me.
Saw the output from the test and should be able to use something like jq
to correctly summarize compaction info based on table or queue.
[
{
"ecid": "ECID:2332d2fd-a26c-4014-a48b-9a4871baafbc",
"addr": "ip-10-113-15-93.evoforge.org:9133",
"kind": "USER",
"queueName": "DCQ7",
"ke": "47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU\u003d",
"tableId": "1",
"status": "STARTED",
"lastUpdate": 806,
"duration": 1,
"numFiles": 1,
"progress": 0.0
}
]
Added option to print the running compactions as json. Co-authored-by: Kevin Rathbun <[email protected]> Co-authored-by: Dave Marion <[email protected]>
This PR enhances the accumulo ec-admin running command by adding a --format flag to support structured output in CSV and JSON formats. This allows for easier programmatic parsing of running compaction details.
Changes Introduced:
Added --format flag (json, csv, human [default]) to ec-admin running.
Implemented structured output formatting for better readability and parsing.
Updated runningCompactions() method to handle different output formats.
Ensured compatibility with existing behavior by keeping table format as the default.
Fixes #5205