smengcl commented on PR #5920: URL: https://github.com/apache/ozone/pull/5920#issuecomment-1879458118
Thanks @errose28 for the comment. > * 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. +1 this. But in this way we'd also need to rename all other commands (e.g. `getserviceroles`, `cancelprepare`). I'd like @jojochuang and @adoroszlai 's opinions as well. > * 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`. This is basically a command that enumerates `OpenKeyTable` (or `OpenFileTable`). It is not just limited to hsync'ed keys but all open keys. I should remove `[hsync]` from the jira title. I do like the idea of `ozone admin key list --open`. > * 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? Yup it is supposed to work with FSO/OBS/LEGACY. Path to a bucket (or just a key prefix) can be optionally passed to limit the scope. But without it (or when passing root as the path) it just iterates over two `OpenKey(File)Table`s. > * 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. I planned to. But it is not finalized yet so I will do that later. Plus I will add it to the markdown doc as well. > * 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. I think we need both. -- 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]
