Akanksha-kedia commented on code in PR #18716:
URL: https://github.com/apache/pinot/pull/18716#discussion_r3419846866
##########
pinot-tools/src/main/java/org/apache/pinot/tools/PinotNumReplicaChanger.java:
##########
@@ -89,6 +89,9 @@ private IdealState updateIdealState(IdealState idealState,
int newNumReplicas) {
Set<String> segmentIds = idealState.getPartitionSet();
for (String segmentId : segmentIds) {
Map<String, String> instanceStateMap =
idealState.getInstanceStateMap(segmentId);
+ if (instanceStateMap == null) {
Review Comment:
Good question. `getInstanceStateMap(segmentId)` delegates to
`ZNRecord.getMapField(segmentId)` which simply does `mapFields.get(segmentId)`.
The value can be `null` in two cases:
1. **ZK data inconsistency**: Helix's `ZNRecord.setMapField(key, null)` is a
valid API call, so a segment can appear in the partition set (via
`getPartitionSet()` → `mapFields.keySet()`) while having a null value stored.
This can occur due to partial ZK writes or data corruption.
2. **Non-callback dry-run path**: When this method is invoked in dry-run
mode (line 69), the `currentIdealState` is fetched directly via
`getResourceIdealState()`. While Pinot itself never writes null maps, an
external Helix operation or ZK inconsistency could produce one.
The guard is a defensive check to avoid a silent NPE at
`instanceStateMap.size()`. I can add a `LOGGER.warn` on the null branch to make
it visible if preferred — let me know.
--
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]
