Copilot commented on code in PR #17827:
URL: https://github.com/apache/pinot/pull/17827#discussion_r2903113439
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotInstanceAssignmentRestletResourceStatelessTest.java:
##########
@@ -142,7 +142,7 @@ public void testInstanceAssignment()
new InstanceReplicaGroupPartitionConfig(false, 0, 0, 0, 0, 0, false,
null), null, false);
realtimeTableConfig.setInstanceAssignmentConfigMap(
Collections.singletonMap(InstancePartitionsType.CONSUMING.toString(),
consumingInstanceAssignmentConfig));
- _helixResourceManager.setExistingTableConfig(realtimeTableConfig);
+ _helixResourceManager.setExistingTableConfig(realtimeTableConfig, -1,
false);
Review Comment:
This call hard-codes `expectedVersion = -1` (ignore version check) and
`force = false`. With the convenience overload removed, using named constants
(or local variables) would make the test intent clearer and reduce repetition.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/periodictask/RealtimeOffsetAutoResetKafkaHandler.java:
##########
@@ -84,11 +83,8 @@ public boolean triggerBackfillJob(String tableNameWithType,
StreamConfig streamC
// Add the new topic to the table config.
TableConfig currentTableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
if (getOrAddBackfillTopic(newTopicStreamConfig, currentTableConfig)) {
- _pinotHelixResourceManager.setExistingTableConfig(currentTableConfig);
+ _pinotHelixResourceManager.setExistingTableConfig(currentTableConfig,
-1, false);
Review Comment:
This call now hard-codes `expectedVersion = -1` (ignore version check) and
`force = false`. Since the overload that hid these defaults was removed,
consider introducing named constants (e.g., `IGNORE_VERSION_CHECK = -1`,
`FORCE_UPDATE = false`) to avoid magic values and clarify intent at the call
site.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotInstanceAssignmentRestletResourceStatelessTest.java:
##########
@@ -188,7 +188,7 @@ public void testInstanceAssignment()
put(InstancePartitionsType.CONSUMING.toString(),
consumingInstanceAssignmentConfig);
put(InstancePartitionsType.COMPLETED.toString(),
offlineInstanceAssignmentConfig);
}});
- _helixResourceManager.setExistingTableConfig(realtimeTableConfig);
+ _helixResourceManager.setExistingTableConfig(realtimeTableConfig, -1,
false);
Review Comment:
Another repeated `setExistingTableConfig(..., -1, false)` call. Consider
replacing `-1`/`false` with named constants to make it obvious you're
intentionally skipping the version check and not forcing backward-incompatible
changes.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotInstanceAssignmentRestletResourceStatelessTest.java:
##########
@@ -172,7 +172,7 @@ public void testInstanceAssignment()
instanceAssignmentConfigMap.put(InstancePartitionsType.OFFLINE.toString(),
offlineInstanceAssignmentConfig);
instanceAssignmentConfigMap.put(TIER_NAME, tierInstanceAssignmentConfig);
offlineTableConfig.setInstanceAssignmentConfigMap(instanceAssignmentConfigMap);
- _helixResourceManager.setExistingTableConfig(offlineTableConfig);
+ _helixResourceManager.setExistingTableConfig(offlineTableConfig, -1,
false);
Review Comment:
Another use of magic `-1` for `expectedVersion` (ignore version check).
Suggest factoring out a constant in this test class to avoid repeating the
sentinel value in multiple assertions/setup steps.
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotInstanceAssignmentRestletResourceStatelessTest.java:
##########
@@ -124,7 +124,7 @@ public void testInstanceAssignment()
new InstanceReplicaGroupPartitionConfig(false, 0, 0, 0, 0, 0, false,
null), null, false);
offlineTableConfig.setInstanceAssignmentConfigMap(
Collections.singletonMap(InstancePartitionsType.OFFLINE.toString(),
offlineInstanceAssignmentConfig));
- _helixResourceManager.setExistingTableConfig(offlineTableConfig);
+ _helixResourceManager.setExistingTableConfig(offlineTableConfig, -1,
false);
Review Comment:
This call now passes `-1` for `expectedVersion` (ignore version check).
Consider using a named constant in the test (e.g., `IGNORE_VERSION_CHECK = -1`)
so the intent is clear and the value isn't duplicated across multiple calls.
--
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]
