Jackie-Jiang commented on code in PR #17380:
URL: https://github.com/apache/pinot/pull/17380#discussion_r2684105554
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java:
##########
@@ -182,6 +182,22 @@ public void setDataCrc(long dataCrc) {
setNonNegativeValue(Segment.DATA_CRC, dataCrc);
}
+ public boolean isUseDataCrcForReplace() {
+ String useDataCrcForReplaceString =
_simpleFields.get(Segment.USE_DATA_CRC_FOR_REPLACE);
+ return useDataCrcForReplaceString != null &&
Boolean.parseBoolean(useDataCrcForReplaceString);
+ }
+
+ // useDataCrcForReplace is set for consuming segments in realtime table
+ // that signal replica server to use Data CRC when available for doing any
replacement
+ // of segments
+ public void setUseDataCrcForReplace(boolean useDataCrcForReplace) {
+ if (useDataCrcForReplace) {
+ _simpleFields.put(Segment.USE_DATA_CRC_FOR_REPLACE,
String.valueOf(useDataCrcForReplace));
Review Comment:
(minor)
```suggestion
_simpleFields.put(Segment.USE_DATA_CRC_FOR_REPLACE, "true");
```
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadata.java:
##########
@@ -182,6 +182,22 @@ public void setDataCrc(long dataCrc) {
setNonNegativeValue(Segment.DATA_CRC, dataCrc);
}
+ public boolean isUseDataCrcForReplace() {
+ String useDataCrcForReplaceString =
_simpleFields.get(Segment.USE_DATA_CRC_FOR_REPLACE);
+ return useDataCrcForReplaceString != null &&
Boolean.parseBoolean(useDataCrcForReplaceString);
Review Comment:
(minor)
```suggestion
return Boolean.parseBoolean(useDataCrcForReplaceString);
```
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1659,7 +1659,18 @@ private SegmentDirectory initSegmentDirectory(String
segmentName, String segment
return segmentDirectoryLoader.load(indexDir.toURI(), loaderContext);
}
+ // CRC check can be performed on both segment CRC and data CRC (if
available) based on the ZK property value of
+ // useDataCRCForReplace.
private static boolean hasSameCRC(SegmentZKMetadata zkMetadata,
SegmentMetadata localMetadata) {
+ if (zkMetadata.isUseDataCrcForReplace()) {
Review Comment:
(minor)
```suggestion
if (zkMetadata.getCrc() == Long.parseLong(localMetadata.getCrc())) {
return true;
}
return zkMetadata.isUseDataCrcForReplace() && zkMetadata.getDataCrc() >=
0 && ...;
```
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadataUtils.java:
##########
@@ -94,6 +94,8 @@ private static void updateSegmentZKMetadata(String
tableNameWithType, SegmentZKM
segmentZKMetadata.setEndOffset(endOffset);
segmentZKMetadata.setStatus(CommonConstants.Segment.Realtime.Status.DONE);
+ // for committing segments, we use data CRC for replace
+ segmentZKMetadata.setUseDataCrcForReplace(true);
Review Comment:
Consider checking data crc exist, then set this flag
We also need to set this flag to false when segment is uploaded
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1782,6 +1782,7 @@ public static class Offline {
public static final String TOTAL_DOCS = "segment.total.docs";
public static final String CRC = "segment.crc";
public static final String DATA_CRC = "segment.data.crc";
+ public static final String USE_DATA_CRC_FOR_REPLACE =
"segment.useDataCRCForReplace";
Review Comment:
+1. Maybe `segment.use.data.crc`
--
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]
