PHOENIX-7239: Fix issues when data and index have different salt buckets #1969
Conversation
…or mutations enerated in CDC index.
…C DF markers. Also refactored some debug code.
| @@ -470,6 +471,7 @@ private IndexMaintainer(final PTable dataTable, final PTable cdcTable, final PTa | |||
| this.immutableStorageScheme = index.getImmutableStorageScheme() == null ? ImmutableStorageScheme.ONE_CELL_PER_COLUMN : index.getImmutableStorageScheme(); | |||
| this.dataEncodingScheme = dataTable.getEncodingScheme() == null ? QualifierEncodingScheme.NON_ENCODED_QUALIFIERS : dataTable.getEncodingScheme(); | |||
| this.dataImmutableStorageScheme = dataTable.getImmutableStorageScheme() == null ? ImmutableStorageScheme.ONE_CELL_PER_COLUMN : dataTable.getImmutableStorageScheme(); | |||
| this.nDataTableSaltBuckets = isDataTableSalted ? dataTable.getBucketNum() : PTable.NO_SALTING; | |||
There was a problem hiding this comment.
Any reason why are you introducing the special value of -1 ? The field nIndexSaltBuckets is set to 0 if index is not salted. We should maintain consistency.
There was a problem hiding this comment.
In the existing code, PTableImpl.toProto() translates 0 into -1 and PTableImpl.createFromProto() leaves this value as null by checking for -1. To be consistent, I think we should change nIndexSaltBuckets to also -1 instead of the other way around. I did a quick check, I think the existing code will work as is, as it checks for > 0. WDYT?
| @@ -1789,6 +1788,9 @@ public static IndexMaintainer fromProto(ServerCachingProtos.IndexMaintainer prot | |||
| } else { | |||
| maintainer.isCDCIndex = false; | |||
| } | |||
| if (proto.hasDataTableSaltBuckets()) { | |||
| maintainer.nDataTableSaltBuckets = proto.getDataTableSaltBuckets(); | |||
There was a problem hiding this comment.
If the proto doesn't have data table salt buckets then what is the nDataTableSaltBuckets field ? I think it will be 0 not -1. Seems like -1 is constant that is introduced is unnecessary.
There was a problem hiding this comment.
Agree on the inconsistency, but I think it should be set to -1 to be consistent than 0 for the same reason I mentioned in the other thread.
|
@haridsv It would have been better if you only had salt bucket fix in this PR and not CDC related changes. |
This PR includes the following changes: