Copilot commented on code in PR #17049:
URL: https://github.com/apache/pinot/pull/17049#discussion_r2449815720
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java:
##########
@@ -1385,6 +1392,55 @@ public void cleanup() {
getHelixResourceManager().deleteSchema(schemaName);
}
}
+
+ // Ensure cluster is purely empty before returning
+ try {
+ TestUtils.waitForCondition(aVoid -> {
+ boolean noTables =
CollectionUtils.isEmpty(_helixResourceManager.getAllTables());
+ boolean noLogicalTables =
CollectionUtils.isEmpty(_helixResourceManager.getAllLogicalTableNames());
+ boolean noSchemas =
CollectionUtils.isEmpty(_helixResourceManager.getAllSchemaNames());
+
+ // Helix: ensure no table IdealState or ExternalView remains
+ boolean noTableResourcesInIdealState =
_helixDataAccessor.getChildNames(_helixDataAccessor.keyBuilder()
+
.idealStates()).stream().noneMatch(TableNameBuilder::isTableResource);
+ boolean noTableResourcesInExternalView =
_helixDataAccessor.getChildNames(_helixDataAccessor.keyBuilder()
+
.externalViews()).stream().noneMatch(TableNameBuilder::isTableResource);
+
+ // Property store: ensure no segments or table-config nodes remain
+ boolean noSegmentsNodes =
CollectionUtils.isEmpty(_propertyStore.getChildNames(
+ "/SEGMENTS", AccessOption.PERSISTENT));
+ boolean noTableConfigNodes =
CollectionUtils.isEmpty(_propertyStore.getChildNames(
+ "/CONFIGS/TABLE", AccessOption.PERSISTENT));
Review Comment:
The PropertyStore `getChildNames()` calls may return null instead of an
empty list when the path doesn't exist or has no children. Using
`CollectionUtils.isEmpty()` on a null value will return true, but this could
mask errors. Consider explicitly checking for null or using a helper method
that handles null safely.
```suggestion
List<String> segmentsChildren =
_propertyStore.getChildNames("/SEGMENTS", AccessOption.PERSISTENT);
boolean noSegmentsNodes = segmentsChildren == null ||
segmentsChildren.isEmpty();
List<String> tableConfigChildren =
_propertyStore.getChildNames("/CONFIGS/TABLE", AccessOption.PERSISTENT);
boolean noTableConfigNodes = tableConfigChildren == null ||
tableConfigChildren.isEmpty();
```
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java:
##########
@@ -1385,6 +1392,55 @@ public void cleanup() {
getHelixResourceManager().deleteSchema(schemaName);
}
}
+
+ // Ensure cluster is purely empty before returning
+ try {
+ TestUtils.waitForCondition(aVoid -> {
+ boolean noTables =
CollectionUtils.isEmpty(_helixResourceManager.getAllTables());
+ boolean noLogicalTables =
CollectionUtils.isEmpty(_helixResourceManager.getAllLogicalTableNames());
+ boolean noSchemas =
CollectionUtils.isEmpty(_helixResourceManager.getAllSchemaNames());
+
+ // Helix: ensure no table IdealState or ExternalView remains
+ boolean noTableResourcesInIdealState =
_helixDataAccessor.getChildNames(_helixDataAccessor.keyBuilder()
+
.idealStates()).stream().noneMatch(TableNameBuilder::isTableResource);
+ boolean noTableResourcesInExternalView =
_helixDataAccessor.getChildNames(_helixDataAccessor.keyBuilder()
+
.externalViews()).stream().noneMatch(TableNameBuilder::isTableResource);
+
+ // Property store: ensure no segments or table-config nodes remain
+ boolean noSegmentsNodes =
CollectionUtils.isEmpty(_propertyStore.getChildNames(
+ "/SEGMENTS", AccessOption.PERSISTENT));
+ boolean noTableConfigNodes =
CollectionUtils.isEmpty(_propertyStore.getChildNames(
+ "/CONFIGS/TABLE", AccessOption.PERSISTENT));
+
+ return noTables && noLogicalTables && noSchemas
+ && noTableResourcesInIdealState && noTableResourcesInExternalView
+ && noSegmentsNodes && noTableConfigNodes;
+ }, 60_000L, "Failed to fully clean up cluster state");
Review Comment:
The cleanup verification logic makes multiple potentially expensive calls on
every poll iteration. Consider caching intermediate results or reducing the
polling frequency. Currently, this polls `getAllTables()`,
`getAllLogicalTableNames()`, `getAllSchemaNames()`, and multiple
Helix/PropertyStore operations repeatedly until the condition is met, which
could be inefficient during the 60-second timeout period.
--
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]
