Reduce PinotHelixResourceManager public APIs#17827
Reduce PinotHelixResourceManager public APIs#17827krishan1390 wants to merge 2 commits intoapache:masterfrom
Conversation
| * @throws IOException | ||
| * @throws TableConfigBackwardIncompatibleException if config changes are backward incompatible | ||
| */ | ||
| public void updateTableConfig(TableConfig tableConfig) |
There was a problem hiding this comment.
I feel this method is more concise. Alternatively we can make it final to prevent wrong override. Directly removing it is backward incompatible
| * @throws IOException | ||
| * @throws TableConfigBackwardIncompatibleException if config changes are backward incompatible and force is false | ||
| */ | ||
| public void updateTableConfig(TableConfig tableConfig, boolean force) |
There was a problem hiding this comment.
Seems it won't throw IOException as well. Let's clean it up
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17827 +/- ##
============================================
+ Coverage 63.20% 63.26% +0.05%
Complexity 1456 1456
============================================
Files 3188 3189 +1
Lines 191736 191760 +24
Branches 29347 29354 +7
============================================
+ Hits 121178 121308 +130
+ Misses 61067 60971 -96
+ Partials 9491 9481 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR simplifies PinotHelixResourceManager’s public surface area by removing redundant overloads for updateTableConfig and setExistingTableConfig, updating controller code and tests to use the remaining canonical method signatures.
Changes:
- Removed
PinotHelixResourceManager.updateTableConfig(TableConfig)overload (keepupdateTableConfig(TableConfig, boolean force)). - Removed
PinotHelixResourceManager.setExistingTableConfigoverloads (keepsetExistingTableConfig(TableConfig, int expectedVersion, boolean force)). - Updated controller logic and test call sites to pass the new required parameters.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java | Removes redundant public overloads, leaving one canonical API per operation. |
| pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/periodictask/RealtimeOffsetAutoResetKafkaHandler.java | Updates call to setExistingTableConfig with explicit args; removes now-unneeded IOException handling. |
| pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java | Updates table cleanup flow to call updateTableConfig(tableConfig, false). |
| pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java | Updates test call sites to use updateTableConfig(tableConfig, false). |
| pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerStatelessTest.java | Updates test call site to use updateTableConfig(tableConfig, false). |
| pinot-controller/src/test/java/org/apache/pinot/controller/helix/TableCacheTest.java | Updates test call site to use updateTableConfig(tableConfig, false). |
| pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotInstanceAssignmentRestletResourceStatelessTest.java | Updates test call sites to use setExistingTableConfig(tableConfig, -1, false). |
| realtimeTableConfig.setInstanceAssignmentConfigMap( | ||
| Collections.singletonMap(InstancePartitionsType.CONSUMING.toString(), consumingInstanceAssignmentConfig)); | ||
| _helixResourceManager.setExistingTableConfig(realtimeTableConfig); | ||
| _helixResourceManager.setExistingTableConfig(realtimeTableConfig, -1, false); |
There was a problem hiding this 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.
| instanceAssignmentConfigMap.put(TIER_NAME, tierInstanceAssignmentConfig); | ||
| offlineTableConfig.setInstanceAssignmentConfigMap(instanceAssignmentConfigMap); | ||
| _helixResourceManager.setExistingTableConfig(offlineTableConfig); | ||
| _helixResourceManager.setExistingTableConfig(offlineTableConfig, -1, false); |
There was a problem hiding this 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.
| put(InstancePartitionsType.COMPLETED.toString(), offlineInstanceAssignmentConfig); | ||
| }}); | ||
| _helixResourceManager.setExistingTableConfig(realtimeTableConfig); | ||
| _helixResourceManager.setExistingTableConfig(realtimeTableConfig, -1, false); |
There was a problem hiding this 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.
| TableConfig currentTableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType); | ||
| if (getOrAddBackfillTopic(newTopicStreamConfig, currentTableConfig)) { | ||
| _pinotHelixResourceManager.setExistingTableConfig(currentTableConfig); | ||
| _pinotHelixResourceManager.setExistingTableConfig(currentTableConfig, -1, false); |
There was a problem hiding this 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.
| offlineTableConfig.setInstanceAssignmentConfigMap( | ||
| Collections.singletonMap(InstancePartitionsType.OFFLINE.toString(), offlineInstanceAssignmentConfig)); | ||
| _helixResourceManager.setExistingTableConfig(offlineTableConfig); | ||
| _helixResourceManager.setExistingTableConfig(offlineTableConfig, -1, false); |
There was a problem hiding this 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.
There are currently 2 Public APIs of updateTableConfig and 3 APIs of setExistingTableConfig.
This makes overriding brittle and also makes clients harder to understand which API to call, so simplifying by keeping just one API for each method.