Jackie-Jiang commented on code in PR #15438:
URL: https://github.com/apache/pinot/pull/15438#discussion_r2025681519
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -621,7 +621,7 @@ 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 IN_PROGRESS");
Review Comment:
We can move this log after creating the new segment metadata, but we still
want to log the segment name
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1066,13 +1066,14 @@ public void segmentStoppedConsuming(LLCSegmentName
llcSegmentName, String instan
String realtimeTableName =
TableNameBuilder.REALTIME.tableNameWithType(llcSegmentName.getTableName());
String segmentName = llcSegmentName.getSegmentName();
- LOGGER.info("Marking CONSUMING segment: {} OFFLINE on instance: {}",
segmentName, instanceName);
+
try {
HelixHelper.updateIdealState(_helixManager, realtimeTableName,
idealState -> {
assert idealState != null;
Map<String, String> stateMap =
idealState.getInstanceStateMap(segmentName);
String state = stateMap.get(instanceName);
Review Comment:
Not introduced in this PR, but we should perform a null check on `stateMap`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -1066,13 +1066,14 @@ public void segmentStoppedConsuming(LLCSegmentName
llcSegmentName, String instan
String realtimeTableName =
TableNameBuilder.REALTIME.tableNameWithType(llcSegmentName.getTableName());
String segmentName = llcSegmentName.getSegmentName();
- LOGGER.info("Marking CONSUMING segment: {} OFFLINE on instance: {}",
segmentName, instanceName);
+
Review Comment:
I feel we should keep it here since we are attempting to mark it OFFLINE
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -634,6 +634,8 @@ public File downloadSegment(SegmentZKMetadata zkMetadata)
Thread.sleep(sleepTimeMs);
}
+ _logger.error("Failed to download segment: {} after: {}ms.", segmentName,
downloadTimeoutMs);
Review Comment:
Can you check if the failure is logged in the helix log? Usually we only log
the error when handling the exception
--
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
