Copilot commented on code in PR #17380:
URL: https://github.com/apache/pinot/pull/17380#discussion_r2625108669
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1281,9 +1282,13 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata
zkMetadata, IndexLoading
closeSegmentDirectoryQuietly(segmentDirectory);
return false;
}
- if (isSegmentStatusCompleted(zkMetadata) && !hasSameCRC(zkMetadata,
segmentMetadata)) {
- _logger.warn("Segment: {} has CRC changed from: {} to: {}", segmentName,
segmentMetadata.getCrc(),
- zkMetadata.getCrc());
+ if (isSegmentStatusCompleted(zkMetadata) && !hasSameCRC(zkMetadata,
segmentMetadata, true)) {
+ _logger.warn("Segment: {} has CRC changed from: {} to: {} and data CRC
prev value: {} new value: {}",
+ segmentName,
+ segmentMetadata.getCrc(),
+ zkMetadata.getCrc(),
+ segmentMetadata.getDataCrc(),
+ zkMetadata.getDataCrc());
Review Comment:
The log message uses positional parameters but the order is inconsistent
with the description. 'from: {} to: {}' should show old then new, but the code
logs local metadata first, then ZK metadata. Consider reordering to 'from: {}
(local) to: {} (ZK)' for clarity, or swap the order of getCrc() calls to match
the 'from→to' semantic.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1659,8 +1664,17 @@ private SegmentDirectory initSegmentDirectory(String
segmentName, String segment
return segmentDirectoryLoader.load(indexDir.toURI(), loaderContext);
}
- private static boolean hasSameCRC(SegmentZKMetadata zkMetadata,
SegmentMetadata localMetadata) {
- return zkMetadata.getCrc() == Long.parseLong(localMetadata.getCrc());
+ // CRC check can be performed on both segment CRC and data CRC (if
available) based on the flag passed
+ private static boolean hasSameCRC(SegmentZKMetadata zkMetadata,
SegmentMetadata localMetadata,
+ boolean useDataCrcIfAvailable) {
+ if (zkMetadata.getCrc() == Long.parseLong(localMetadata.getCrc())) {
+ return true;
+ } else if (useDataCrcIfAvailable) {
+ return zkMetadata.getDataCrc() >= 0
+ && Long.parseLong(localMetadata.getDataCrc()) >= 0
+ && zkMetadata.getDataCrc() ==
Long.parseLong(localMetadata.getDataCrc());
Review Comment:
This code may throw NumberFormatException if `localMetadata.getDataCrc()`
returns a non-numeric string. The check should validate that the string is
parseable before attempting to parse it, or the method should use a try-catch
block to handle potential parsing failures gracefully.
```suggestion
if (zkMetadata.getDataCrc() < 0) {
return false;
}
String localDataCrc = localMetadata.getDataCrc();
if (!StringUtils.isNumeric(localDataCrc)) {
LOGGER.warn("Local segment metadata data CRC is not numeric: {}",
localDataCrc);
return false;
}
long localDataCrcValue;
try {
localDataCrcValue = Long.parseLong(localDataCrc);
} catch (NumberFormatException e) {
// Guard against values that are numeric but outside the range of a
long
LOGGER.warn("Failed to parse local segment metadata data CRC: {}",
localDataCrc, e);
return false;
}
if (localDataCrcValue < 0) {
return false;
}
return zkMetadata.getDataCrc() == localDataCrcValue;
```
--
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]
