xiangfu0 commented on code in PR #20113:
URL: https://github.com/apache/pinot/pull/20113#discussion_r4434870915
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1570,6 +1570,12 @@ public static class Server {
public static final String SEGMENT_FORMAT_VERSION =
"segment.format.version";
public static final String CONFIG_OF_SEGMENT_FORMAT_VERSION =
INSTANCE_DATA_MANAGER_CONFIG_PREFIX + "." + SEGMENT_FORMAT_VERSION;
+ // Version of the data CRC written into newly built segments. Replicas
compare data CRCs at commit, so keep it the
Review Comment:
Mixed v1/v2 replicas can still *match*, not just miss. v2 equals v1 when a
copy has no null vector. So a v2 copy without nulls and a v1 copy with nulls
(or the reverse) get the same data CRC, and `hasSameCRC` keeps the divergent
copy. I checked this locally: `hasSameCRC(zk = v2 no-null, local = v1
with-null) == true`. That is no worse than master, but the fix only works once
every replica builds v2. The "fall back to the full CRC" statement in the PR
description doesn't hold for this case. Consider documenting it here, or
recording the version in ZK and requiring it to match.
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -306,6 +313,63 @@ public void
testDataCrcNotGeneratedIfForwardIndexDisabled() throws Exception {
}
}
+ @Test
+ public void testDataCrcVersions()
+ throws Exception {
+ // A null is stored as the default value (0) in the forward index, so only
the null vector tells these apart
+ SegmentMetadataImpl withNullV1 = buildNullableSegment(null, "withNullV1");
+ SegmentMetadataImpl withDefaultV1 = buildNullableSegment(0,
"withDefaultV1");
+ SegmentMetadataImpl noNullV1 = buildNullableSegment(7, "noNullV1");
+ assertEquals(withNullV1.getDataCrcVersion(),
V1Constants.DataCrc.VERSION_1);
+ assertEquals(withDefaultV1.getDataCrc(), withNullV1.getDataCrc());
+ // Version 1 segments keep the creation.meta layout from before
versioning: CRC, creation time and data CRC
+
assertEquals(SegmentDirectoryPaths.findCreationMetaFile(withNullV1.getIndexDir()).length(),
3 * Long.BYTES);
+
+ CrcUtils.setDefaultDataCrcVersion(V1Constants.DataCrc.VERSION_2);
+ try {
+ SegmentMetadataImpl withNullV2 = buildNullableSegment(null,
"withNullV2");
+ SegmentMetadataImpl withNullAgainV2 = buildNullableSegment(null,
"withNullAgainV2");
+ SegmentMetadataImpl withDefaultV2 = buildNullableSegment(0,
"withDefaultV2");
+ SegmentMetadataImpl noNullV2 = buildNullableSegment(7, "noNullV2");
+ assertEquals(withNullV2.getDataCrcVersion(),
V1Constants.DataCrc.VERSION_2);
+ assertEquals(withNullAgainV2.getDataCrc(), withNullV2.getDataCrc());
+ assertNotEquals(withDefaultV2.getDataCrc(), withNullV2.getDataCrc());
+ // Without null vectors, version 2 matches version 1, so mixed-version
replicas still match
+ assertEquals(noNullV2.getDataCrc(), noNullV1.getDataCrc());
Review Comment:
Optional: also assert `withDefaultV2.getDataCrc() ==
withNullV1.getDataCrc()` to pin down the mixed-version behavior above. A case
that goes through `BaseTableDataManager.hasSameCRC` would also cover the
replica keep/download decision this PR is fixing.
--
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]
