Jackie-Jiang commented on code in PR #15438:
URL: https://github.com/apache/pinot/pull/15438#discussion_r2032187344
##########
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:
I think we should still keep it for debugging purpose. When table is paused,
the following log is skipped
##########
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:
Assert is not correct. If it is `null` we should simply skip it. It is
possible when the segment is already deleted.
Assert should be used only for annotation purpose to let both future
developer and IDE know the field is not possible to be `null`. If it is
possible, but should not proceed, we should use `Precondition` because assert
is ignored in production environment.
##########
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:
Consider adding the error log in `SegmentOnlineOfflineStateModelFactory`
similar to how the errors are handled in
`BrokerResourceOnlineOfflineStateModelFactory`. There are other errors only
logged in helix log but not in server log
--
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
