HDDS-8188. Support max allowed length in response of ozone admin container list #6497
Conversation
|
cc. @errose28 |
|
Please see HDDS-8887 for similar functionality implemented in a more user-friendly way in other list commands. |
…ocolClientSideTranslatorPB class
|
It's good to have an "all" option, but I don't think we should make it default, as in a production cluster, there will be hundreds of thousands of containers there. |
|
The entire protobuf message has to be staged on the server before it is returned to the client, right? So what is the overhead in memory of making this the default with millions of containers in the system? I don't think listing all should be the default, and if you want it to be, the client should bring them down from the server in pages and make it invisible to the end user. |
|
Agreed with Stephen and Sammi, if we have to load all the containers into one proto and send it over that is probably not optimal. When I suggested this should be the default last year I think I was imagining something like our If the SCM doesn't do this already then that might be a change for a different PR, and here we can just add a non-default |
|
For context, |
Galsza
left a comment
Galsza
left a comment
There was a problem hiding this comment.
Alternatively you could also do what ListSubcommand does for certificates, where the results are limited by a batch count and if the user tries asking for more than the limit, they get a warning message and an option to list more.
… stdout indicating the result is truncated
|
Added a ScmConfigKeys#hdds.container.list.max.count to allow the user to configure themselves the max count of containers allowed to be included in list-container operation's response. If the result is over the max count allowed, printing a message to stdout indicating the result is truncated. To view the rest of container list, need to implement pagination (https://issues.apache.org/jira/browse/HDDS-10687) for list-container operation. |
DaveTeng0
changed the title
|
hello team~ feel free to let me know if there is any further question about this pr. Thanks! (cc. @ChenSammi @Galsza @errose28 |
...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
Outdated
Show resolved
Hide resolved
Galsza
left a comment
Galsza
left a comment
There was a problem hiding this comment.
Sorry for getting back late, I've left two minor comments otherwise LGTM
yeah! refactored two listContainer methods, to make them reuse common code as much as possible. Thanks @Galsza for the suggestion! |
|
Hello! If no further new comments, please feel free to merge! Thanks! |
|
Hello @Galsza ! would you mind granting an approval for this pr if there is no further questions? Thanks! |
Galsza
left a comment
Galsza
left a comment
There was a problem hiding this comment.
Thank you for the changes @DaveTeng0 lgtm
|
Hello! could someone help merge this PR? Thanks! |
| * @throws IOException | ||
| */ | ||
| List<ContainerInfo> listContainer(long startContainerID, | ||
| Pair<List<ContainerInfo>, Long> listContainer(long startContainerID, |
There was a problem hiding this comment.
This kind of change doesn't seem to be backwards compatible.
|
Thanks @DaveTeng0 for working on this. Continued in #7181. |
6 participants
What changes were proposed in this pull request?
ozone admin container list has a --count parameter that defaults to 20. This has caused confusion among users, and it does not provide a way to list all containers. We should support --all to list all containers(within max allowed count of 'hdds.container.list.max.count'. If the total count exceeds max allowed count, CLI prints a message to stdout indicating the result is truncated.)
(To view the full list of container, need to implement pagination https://issues.apache.org/jira/browse/HDDS-10687 for list-container operation.)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8188
How was this patch tested?
unit test.