[GitHub] [ozone] GlenGeng commented on a change in pull request #2044: HDDS-4968. Back-port HDDS-4911 (List container by container state) to ContainerManagerV2

[GitBox <gi...@apache.org>]

GlenGeng commented on a change in pull request #2044:
URL: https://github.com/apache/ozone/pull/2044#discussion_r594823572



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
##########
@@ -167,12 +167,40 @@ public ContainerInfo getContainer(final ContainerID id)
 
   @Override
   public List<ContainerInfo> getContainers(final LifeCycleState state) {
-    return containerStateManager.getContainerIDs(state).stream()
+    final List<ContainerID> containersIds =
+        new ArrayList<>(containerStateManager.getContainerIDs(state));
+    Collections.sort(containersIds);
+    return containersIds.stream()
         .map(ContainerID::getProtobuf)
         .map(containerStateManager::getContainer)
         .filter(Objects::nonNull).collect(Collectors.toList());
   }
 
+  @Override
+  public List<ContainerInfo> listContainer(
+      ContainerID startContainerID,
+      int count, HddsProtos.LifeCycleState state) {
+    lock.lock();

Review comment:
       The lock here is not needed, check the comments
   
   ```
     // Limit the number of on-going ratis operations.
     private final Lock lock;
   ```

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerV2.java
##########
@@ -84,6 +85,43 @@ ContainerInfo getContainer(ContainerID containerID)
    */
   List<ContainerInfo> getContainers(LifeCycleState state);
 
+
+  /**
+   * Returns containers under certain conditions.
+   * Search container IDs from start ID(exclusive),
+   * The max size of the searching range cannot exceed the
+   * value of count.
+   *
+   * @param startContainerID start containerID, >=0,
+   * start searching at the head if 0.
+   * @param count count must be >= 0
+   *              Usually the count will be replace with a very big
+   *              value instead of being unlimited in case the db is very big.
+   * @param state Container of this state will be returned. Can be null.
+   *
+   * @return a list of container.
+   * @throws IOException
+   */
+  List<ContainerInfo> listContainer(ContainerID startContainerID, int count,

Review comment:
       Since we already have ` List<ContainerInfo> getContainers(LifeCycleState state);`, the two interface is not needed, they are duplicate with `getContainers`.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
##########
@@ -167,12 +167,40 @@ public ContainerInfo getContainer(final ContainerID id)
 
   @Override
   public List<ContainerInfo> getContainers(final LifeCycleState state) {
-    return containerStateManager.getContainerIDs(state).stream()
+    final List<ContainerID> containersIds =
+        new ArrayList<>(containerStateManager.getContainerIDs(state));

Review comment:
       stream support sort, this change complicates the code.




----------------------------------------------------------------
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.

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