szetszwo commented on code in PR #10736:
URL: https://github.com/apache/ozone/pull/10736#discussion_r3576861219
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java:
##########
@@ -118,6 +120,30 @@ List<ContainerInfo> getInfos(ContainerID start, int count)
{
.collect(Collectors.toList());
}
+ List<ContainerID> getFilteredContainerIDs(ContainerID start, int count,
+ LifeCycleState lifeCycleState, ContainerHealthState healthState) {
Review Comment:
- lifeCycleState always null.
- Remove "Filtered" from the name
```java
List<ContainerID> getContainerIDs(ContainerID start, int count,
ContainerHealthState healthState) {
Objects.requireNonNull(start, "start == null");
Preconditions.assertTrue(count >= 0, "count < 0");
final List<ContainerID> result = new ArrayList<>(1024);
for (ContainerEntry entry : map.tailMap(start).values()) {
ContainerInfo info = entry.getInfo();
if (healthState == null || info.getHealthState() != healthState) {
result.add(info.containerID());
if (result.size() >= count) {
break;
}
}
}
return result;
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java:
##########
@@ -118,6 +120,30 @@ List<ContainerInfo> getInfos(ContainerID start, int count)
{
.collect(Collectors.toList());
}
+ List<ContainerID> getFilteredContainerIDs(ContainerID start, int count,
+ LifeCycleState lifeCycleState, ContainerHealthState healthState) {
+ Objects.requireNonNull(start, "start == null");
+ Preconditions.assertTrue(count >= 0, "count < 0");
+ List<ContainerID> result = new ArrayList<>(Math.min(count, 64));
Review Comment:
Use 1024
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java:
##########
@@ -106,11 +106,14 @@ public interface ContainerStateManager extends SCMHandler
{
/**
* Get {@link ContainerID}s for the given state.
*
+ * @param state optional lifecycle filter
+ * @param healthState optional health filter
Review Comment:
Use LifeCycleState and ContainerHealthState. The word "filter" is quite
confusing since all the other methods do not have it. Please do not use it.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java:
##########
@@ -118,6 +120,30 @@ List<ContainerInfo> getInfos(ContainerID start, int count)
{
.collect(Collectors.toList());
}
+ List<ContainerID> getFilteredContainerIDs(ContainerID start, int count,
+ LifeCycleState lifeCycleState, ContainerHealthState healthState) {
+ Objects.requireNonNull(start, "start == null");
+ Preconditions.assertTrue(count >= 0, "count < 0");
+ List<ContainerID> result = new ArrayList<>(Math.min(count, 64));
+ if (count == 0) {
+ return result;
+ }
Review Comment:
count == 0 should be checked in the public method.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java:
##########
@@ -274,6 +300,44 @@ public List<ContainerID> getContainerIDs(LifeCycleState
state, ContainerID start
.collect(Collectors.toList());
}
+ private List<ContainerID> getContainerIDsFromLifecycleIndex(LifeCycleState
lifeCycleState,
+ ContainerHealthState healthState, ContainerID start, int count) {
+ List<ContainerID> result = new ArrayList<>(Math.min(count, 64));
+ if (count == 0) {
+ return result;
+ }
+ for (ContainerInfo info : lifeCycleStateMap.tailMap(lifeCycleState,
start).values()) {
+ if (info.getHealthState() == healthState) {
+ result.add(info.containerID());
+ if (result.size() >= count) {
+ break;
+ }
+ }
+ }
+ return result;
+ }
+
+ /**
+ * Returns container IDs matching optional lifecycle and health filters,
+ * in ascending {@link ContainerID} order starting from {@code start}
+ * (inclusive).
+ * Lifecycle-only and lifecycle + health filters use the lifecycle index.
+ * Health-only filters scan the full container map (no health index); prefer
+ * supplying a lifecycle filter at scale.
+ */
+ public List<ContainerID> getContainerIDs(LifeCycleState lifeCycleState,
+ ContainerHealthState healthState, ContainerID start, int count) {
+ Preconditions.assertTrue(count >= 0, "count < 0");
+ if (healthState == null && lifeCycleState != null) {
+ return getContainerIDs(lifeCycleState, start, count);
+ }
+ if (healthState != null && lifeCycleState != null) {
+ return getContainerIDsFromLifecycleIndex(lifeCycleState, healthState,
start, count);
+ }
+ return containerMap.getFilteredContainerIDs(start, count, lifeCycleState,
+ healthState);
+ }
Review Comment:
Remove getContainerIDsFromLifecycleIndex and simplify this method as as
below:
```java
/**
* Returns container IDs matching given optional lifeCycleState and
healthState,
* in ascending {@link ContainerID} order starting from {@code start}
(inclusive).
*
* @param start the start id
* @param count the maximum size of the returned list
* @return a list of sorted {@link ContainerID}s
*/
public List<ContainerID> getContainerIDs(LifeCycleState lifeCycleState,
ContainerHealthState healthState, ContainerID start, int count) {
if (count == 0) {
return Collections.emptyList();
}
Preconditions.assertTrue(count > 0, "count < 0");
if (lifeCycleState == null) {
return containerMap.getContainerIDs(start, count, healthState);
}
final List<ContainerID> result = new ArrayList<>(Math.min(count, 1024));
for (ContainerInfo info : lifeCycleStateMap.tailMap(lifeCycleState,
start).values()) {
if (healthState == null || info.getHealthState() == healthState) {
result.add(info.containerID());
if (result.size() >= count) {
break;
}
}
}
return result;
}
```
--
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]
