You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sarvekshayr (via GitHub)" <gi...@apache.org> on 2024/09/11 07:20:19 UTC
[PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
sarvekshayr opened a new pull request, #7181:
URL: https://github.com/apache/ozone/pull/7181
## 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?
Tested using unit tests.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "sarvekshayr (via GitHub)" <gi...@apache.org>.
sarvekshayr commented on code in PR #7181:
URL: https://github.com/apache/ozone/pull/7181#discussion_r1760950755
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ListSubcommand.java:
##########
@@ -105,12 +119,39 @@ public void execute(ScmClient scmClient) throws IOException {
ReplicationType.fromProto(type),
replication, new OzoneConfiguration());
}
- List<ContainerInfo> containerList =
- scmClient.listContainer(startId, count, state, type, repConfig);
+
+ int maxCountAllowed = parent.getParent().getOzoneConf()
+ .getInt(ScmConfigKeys.HDDS_CONTAINER_LIST_MAX_COUNT,
+ ScmConfigKeys.HDDS_CONTAINER_LIST_MAX_COUNT_DEFAULT);
+ if (all) {
+ System.out.printf("Attempting to list all containers." +
+ " The total number of container might exceed" +
+ " the cluster's current limit of %s. The results will be capped at the" +
+ " maximum allowed count.%n", ScmConfigKeys.HDDS_CONTAINER_LIST_MAX_COUNT_DEFAULT);
Review Comment:
Yes, since these messages warn the user that not all data is being shown due to a restriction; it should be printed to `System.err`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #7181:
URL: https://github.com/apache/ozone/pull/7181#discussion_r1762508398
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -387,12 +388,24 @@ public List<ContainerInfo> listContainer(long startContainerID, int count)
return listContainer(startContainerID, count, null, null, null);
}
+ @Override
Review Comment:
IMO,
We should not provide new method, need update existing in such a way works with both old and new for below scenario,
1) New Client - New Server
2) New Client - Old Server (since proto field is optional, so if old server, has***() check will confirm if field exit or not and can assign default as -1 as not exist)
3) Old Client - New Server (client will ignore new field)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "sarvekshayr (via GitHub)" <gi...@apache.org>.
sarvekshayr commented on PR #7181:
URL: https://github.com/apache/ozone/pull/7181#issuecomment-2348027219
@sumitagrawl @adoroszlai @Galsza
Please review this PR. TIA!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl merged PR #7181:
URL: https://github.com/apache/ozone/pull/7181
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #7181:
URL: https://github.com/apache/ozone/pull/7181#discussion_r1768195010
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ContainerListResult.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.client;
+
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+
+import java.util.List;
+
+/**
+ * Wrapper class for the result of listing containers with their total count.
+ */
+public class ContainerListResult {
Review Comment:
Can we move this class to `hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container` instead? Many of the existing API use classes that are present in there, No other utility classes seem to be present in `hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client`
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #7181:
URL: https://github.com/apache/ozone/pull/7181#issuecomment-2353507591
Hi @sarvekshayr can we do this with pagination as described in HDDS-10687? From what I can tell we don't need a new SCM API to do this since `SCMClientProtocolServer#listContainer` already takes an offset and count. We just need the client to transparently paginate through calls to this API with fixed size batches when the user passes `--all`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #7181:
URL: https://github.com/apache/ozone/pull/7181#issuecomment-2354742105
> Hi @sarvekshayr can we do this with pagination as described in [HDDS-10687](https://issues.apache.org/jira/browse/HDDS-10687)? From what I can tell we don't need a new SCM API to do this since `SCMClientProtocolServer#listContainer` already takes an offset and count. We just need the client to transparently paginate through calls to this API with fixed size batches when the user passes `--all`.
Already listContainer have option for start container (i.e. -s). So pagination for cli can use below way (already present).
Lets, count=10 as used,
Call 1: -s = 0 (or if not provided, will return from first container) ==> 1-10 containers
Call 2: -s=11, it will return next matching containers (10 containers), like from 11-20
OR, if 11-13 is not present, will return 14-23 containers.
So nothing more is required for pagination and can follow above approach. Similar case is implemented in various UI pagination passing starting container IDs.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #7181:
URL: https://github.com/apache/ozone/pull/7181#issuecomment-2356262424
> So nothing more is required for pagination and can follow above approach. Similar case is implemented in various UI pagination passing starting container IDs.
Passing a start ID to the CLI works if the user wants to look at a specific range, but it is bad UX to have this be the only option to get all the containers, because it requires users to manually control the batch size per message. Users should be able to pass `--all` and get all the results while the software regulates message sizes. Maybe some of our other list APIs don't provide this functionality, but really they all should.
Additionally we can support a combination of `--start` and `--all` to allow retrieving all containers after a certain ID. This could also be done with the existing server API and handled on the client side.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-8188. Support max allowed length in response of ozone admin container list [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7181:
URL: https://github.com/apache/ozone/pull/7181#discussion_r1760799678
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ListSubcommand.java:
##########
@@ -105,12 +119,39 @@ public void execute(ScmClient scmClient) throws IOException {
ReplicationType.fromProto(type),
replication, new OzoneConfiguration());
}
- List<ContainerInfo> containerList =
- scmClient.listContainer(startId, count, state, type, repConfig);
+
+ int maxCountAllowed = parent.getParent().getOzoneConf()
+ .getInt(ScmConfigKeys.HDDS_CONTAINER_LIST_MAX_COUNT,
+ ScmConfigKeys.HDDS_CONTAINER_LIST_MAX_COUNT_DEFAULT);
+ if (all) {
+ System.out.printf("Attempting to list all containers." +
+ " The total number of container might exceed" +
+ " the cluster's current limit of %s. The results will be capped at the" +
+ " maximum allowed count.%n", ScmConfigKeys.HDDS_CONTAINER_LIST_MAX_COUNT_DEFAULT);
Review Comment:
Should this and similar informational messages go to `System.err`?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -509,6 +543,104 @@ public List<ContainerInfo> listContainer(long startContainerID,
}
}
+ /**
+ * Lists a range of containers and get their info.
+ *
+ * @param startContainerID start containerID.
+ * @param count count must be {@literal >} 0.
+ * @param state Container with this state will be returned.
+ * @param factor Container factor.
+ * @return a list of containers capped by max count allowed
+ * in "hdds.container.list.max.count" and total number of containers.
+ * @throws IOException
+ */
+ @Override
+ @Deprecated
Review Comment:
Instead of adding a new method that's already deprecated, we can call the one that accepts `ReplicationConfig`:
https://github.com/apache/ozone/blob/6b2f0f2e8e0541d7175093f0acd73fe7fdff7bab/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java#L726-L731
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java:
##########
@@ -128,6 +128,19 @@ void deleteContainer(long containerId, Pipeline pipeline, boolean force)
List<ContainerInfo> listContainer(long startContainerID,
int count) throws IOException;
+ /**
+ * Lists a range of containers and get their info.
+ *
+ * @param startContainerID start containerID.
+ * @param count count must be {@literal >} 0.
+ *
+ * @return a list of containers capped by max count allowed
+ * in "hdds.container.list.max.count" and total number of containers.
+ * @throws IOException
+ */
+ Pair<List<ContainerInfo>, Long> listContainerWithCount(long startContainerID,
+ int count) throws IOException;
Review Comment:
Please consider creating a wrapper object for the return type, instead of using `Pair`. Having a custom type allows it to evolve, if necessary. It also makes the code more readable, by using more specific method names instead of `getLeft`, `getRight`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org