errose28 commented on PR #5920: URL: https://github.com/apache/ozone/pull/5920#issuecomment-1879243590
Just a few comments on the CLI portion of this change: - I think we should follow standard kebab-case conventions for commands going forwards. - We should avoid combining multiple words in one command since it makes it less flexible for future use. Instead of `ozone admin om listopenfiles` we could do something like `ozone admin om open-files ls` in case we need to provide more open file operations in the future. - I'm not sure whether we want to use `open-files` or `open-keys` as the term here. Even though this is being implemented for hsync which is a filesystem operation, the command is under `ozone admin` and therefore outside of ofs. In this area we usually refer to objects as `keys` like `ozone admin key list`. - Related to the above, can you clarify the scope of this command? Is it operating on only open files, open keys as well? Does it work for all bucket layouts? Do you pass it a bucket name to limit the scope? - Can you provide an example of expected CLI parameters and output in the PR description? This makes it easier for others to quickly give feedback on the UI portion without reviewing implementation details. - Since the output will likely be verbose, I think it makes sense to only have this command print json, similar to `ozone sh key list`. In the current draft there is a --json flag that defaults to false. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
