mcvsubbu commented on a change in pull request #7058:
URL: https://github.com/apache/incubator-pinot/pull/7058#discussion_r668903399
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
##########
@@ -539,15 +543,42 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
lock.unlock();
}
- // TODO: also create the new partition groups here, instead of waiting
till the {@link RealtimeSegmentValidationManager} runs
+ // Creates new partition groups here instead of waiting till the {@link
RealtimeSegmentValidationManager} runs
// E.g. If current state is A, B, C, and newPartitionGroupMetadataList
contains B, C, D, E,
- // then create metadata/idealstate entries for D, E along with the
committing partition's entries.
- // Ensure that multiple committing segments don't create multiple new
segment metadata and ideal state entries for the same partitionGroup
+ // then metadata/idealstate entries for D, E are created along with the
committing partition's entries.
+
+ addNewPartitionGroups(realtimeTableName, tableConfig, instancePartitions,
idealState, numReplicas, streamConfig,
+ newPartitionGroupMetadataList, numPartitionGroups, segmentAssignment,
instancePartitionsMap);
// Trigger the metadata event notifier
_metadataEventNotifierFactory.create().notifyOnSegmentFlush(tableConfig);
}
+ /**
+ * Method is kept synchronised so that multiple committing segments don't
create multiple new segment metadata
+ * and ideal state entries for the same partitionGroup
+ */
+ private synchronized void addNewPartitionGroups(String realtimeTableName,
TableConfig tableConfig,
Review comment:
- The code updates ideal state, but it is not written back to zookeeper
anywhere.
- It is possible (under some race conditions) that the background job runs
in another controller. Need to be careful here. Good thing is, I have added
code for idealstate update (PR #6483) so that we don't create two segments with
the same seq number but different timestamp. It covers all existing code path,
but since this is a new code path, you should make sure that is covered. In
that case, the idealstate update will fail with an exception, and you need to
handle it correctly.
--
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]
