Jackie-Jiang commented on code in PR #15438:
URL: https://github.com/apache/pinot/pull/15438#discussion_r2036124534
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -621,11 +621,17 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
preProcessNewSegmentZKMetadata();
// Step-2: Create new segment metadata if needed
- LOGGER.info("Creating new segment metadata with status IN_PROGRESS: {}",
committingSegmentName);
+ LOGGER.info("Creating new segment metadata with status: {}",
Status.IN_PROGRESS);
Review Comment:
Revert this since we do want to log the segment name
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -621,11 +621,17 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
preProcessNewSegmentZKMetadata();
// Step-2: Create new segment metadata if needed
- LOGGER.info("Creating new segment metadata with status IN_PROGRESS: {}",
committingSegmentName);
+ LOGGER.info("Creating new segment metadata with status: {}",
Status.IN_PROGRESS);
long startTimeNs2 = System.nanoTime();
String newConsumingSegmentName =
createNewSegmentMetadata(tableConfig, idealState,
committingSegmentDescriptor, committingSegmentZKMetadata,
Review Comment:
Let's annotate the return for this method to `@Nullable`
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentOnlineOfflineStateModelFactory.java:
##########
@@ -85,7 +85,16 @@ public void onBecomeConsumingFromOffline(Message message,
NotificationContext co
public void onBecomeOnlineFromConsuming(Message message,
NotificationContext context)
throws Exception {
_logger.info("SegmentOnlineOfflineStateModel.onBecomeOnlineFromConsuming() :
{}", message);
- _instanceDataManager.addOnlineSegment(message.getResourceName(),
message.getPartitionName());
+
+ try {
Review Comment:
Let's add it to all state transitions. We can consider making it a separate
PR just for this
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -621,11 +621,17 @@ private void commitSegmentMetadataInternal(String
realtimeTableName,
preProcessNewSegmentZKMetadata();
// Step-2: Create new segment metadata if needed
- LOGGER.info("Creating new segment metadata with status IN_PROGRESS: {}",
committingSegmentName);
+ LOGGER.info("Creating new segment metadata with status: {}",
Status.IN_PROGRESS);
long startTimeNs2 = System.nanoTime();
String newConsumingSegmentName =
createNewSegmentMetadata(tableConfig, idealState,
committingSegmentDescriptor, committingSegmentZKMetadata,
instancePartitions);
+ if (newConsumingSegmentName != null) {
+ LOGGER.info("Created new segment metadata for segment: {} with status:
{}.", newConsumingSegmentName,
+ Status.IN_PROGRESS);
+ } else {
+ LOGGER.info("Skipped creation of new segment metadata as new consuming
segment name is: null");
+ }
Review Comment:
Suggest moving these logs into `createNewSegmentMetadata()` where we have
better context. We don't create new consuming segment because the ingestion is
paused
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
