Skip to content

PHOENIX-7239: Fix issues when data and index have different salt buckets #1969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
haridsv wants to merge 8 commits into from
Closed

PHOENIX-7239: Fix issues when data and index have different salt buckets #1969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ccbc35
@PHOENIX-7239: Save data table salt buckets in IndexMaintainer
haridsv Jul 9, 2024
17bd627
Fix the bug that is causing incorrect salt bucket number to be used f…
haridsv Aug 22, 2024
b32593b
Revamped the time range test and included the coverage for special CD…
haridsv Sep 3, 2024
b66d9e9
Fix issues with debug logging code
haridsv Sep 13, 2024
7f342cd
Uncomment debug code to help diagnose the occasional flappers we are …
haridsv Sep 13, 2024
8e94074
Reverting CDC specific changes to be moved to a separate PR
haridsv Sep 23, 2024
a6ea44e
Make default salt bucket value as -1 for consistency
haridsv Sep 23, 2024
78e51bc
haridsv Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMaintainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ public static IndexMaintainer getIndexMaintainer(List<IndexMaintainer> maintaine
private RowKeyMetaData rowKeyMetaData;
private byte[] indexTableName;
private int nIndexSaltBuckets;
private int nDataTableSaltBuckets;
private byte[] dataEmptyKeyValueCF;
private ImmutableBytesPtr emptyKeyValueCFPtr;
private int nDataCFs;
Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Contributor

@tkhurana tkhurana Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@haridsv haridsv Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@tkhurana tkhurana Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good @haridsv


byte[] indexTableName = index.getPhysicalName().getBytes();
// Use this for the nDataSaltBuckets as we need this for local indexes
Expand Down Expand Up @@ -538,7 +540,7 @@ private IndexMaintainer(final PTable dataTable, final PTable cdcTable, final PTa
this.indexedColumnTypes = Lists.<PDataType>newArrayListWithExpectedSize(nIndexPKColumns-nDataPKColumns);
this.indexedExpressions = Lists.newArrayListWithExpectedSize(nIndexPKColumns-nDataPKColumns);
this.coveredColumnsMap = Maps.newHashMapWithExpectedSize(nIndexColumns - nIndexPKColumns);
this.nIndexSaltBuckets = nIndexSaltBuckets == null ? 0 : nIndexSaltBuckets;
this.nIndexSaltBuckets = nIndexSaltBuckets == null ? PTable.NO_SALTING : nIndexSaltBuckets;
this.dataEmptyKeyValueCF = SchemaUtil.getEmptyColumnFamily(dataTable);
this.emptyKeyValueCFPtr = SchemaUtil.getEmptyColumnFamilyPtr(index);
this.nDataCFs = dataTable.getColumnFamilies().size();
Expand Down Expand Up @@ -918,14 +920,11 @@ public byte[] buildDataRowKey(ImmutableBytesWritable indexRowKeyPtr, byte[][] vi
length--;
trailingVariableWidthColumnNum--;
}
// TODO: need to capture nDataSaltBuckets instead of just a boolean. For now,
// we store this in nIndexSaltBuckets, as we only use this function for local indexes
// in which case nIndexSaltBuckets would never be used. Note that when we do add this
// to be serialized, we have to add it at the end and allow for the value not being
// there to maintain compatibility between an old client and a new server.
if (isDataTableSalted) {
// Set salt byte
byte saltByte = SaltingUtil.getSaltingByte(dataRowKey, SaltingUtil.NUM_SALTING_BYTES, length-SaltingUtil.NUM_SALTING_BYTES, nIndexSaltBuckets);
byte saltByte = SaltingUtil.getSaltingByte(dataRowKey,
SaltingUtil.NUM_SALTING_BYTES, length-SaltingUtil.NUM_SALTING_BYTES,
nDataTableSaltBuckets);
dataRowKey[0] = saltByte;
}
return dataRowKey.length == length ? dataRowKey : Arrays.copyOf(dataRowKey, length);
Expand Down Expand Up @@ -1789,6 +1788,8 @@ public static IndexMaintainer fromProto(ServerCachingProtos.IndexMaintainer prot
} else {
maintainer.isCDCIndex = false;
}
maintainer.nDataTableSaltBuckets = proto.hasDataTableSaltBuckets() ?
proto.getDataTableSaltBuckets() : -1;
maintainer.initCachedState();
return maintainer;
}
Expand Down Expand Up @@ -1933,6 +1934,9 @@ public static ServerCachingProtos.IndexMaintainer toProto(IndexMaintainer mainta
}
}
builder.setIsCDCIndex(maintainer.isCDCIndex);
if (maintainer.isDataTableSalted) {
builder.setDataTableSaltBuckets(maintainer.nDataTableSaltBuckets);
}
return builder.build();
}

Expand Down
1 change: 1 addition & 0 deletions phoenix-core-client/src/main/java/org/apache/phoenix/schema/PTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public interface PTable extends PMetaDataEntity {
public static final String IS_IMMUTABLE_ROWS_PROP_NAME = "IMMUTABLE_ROWS";
public static final boolean DEFAULT_DISABLE_WAL = false;
public static final boolean DEFAULT_IMMUTABLE_ROWS = false;
static final Integer NO_SALTING = -1;

public enum ViewType {
MAPPED((byte)1),
Expand Down
1 change: 0 additions & 1 deletion phoenix-core-client/src/main/java/org/apache/phoenix/schema/PTableImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@
* @since 0.1
*/
public class PTableImpl implements PTable {
private static final Integer NO_SALTING = -1;
private static final int VIEW_MODIFIED_UPDATE_CACHE_FREQUENCY_BIT_SET_POS = 0;
private static final int VIEW_MODIFIED_USE_STATS_FOR_PARALLELIZATION_BIT_SET_POS = 1;
private static final int VIEW_MODIFIED_PHOENIX_TTL_BIT_SET_POS = 2;
Expand Down
1 change: 1 addition & 0 deletions phoenix-core-client/src/main/protobuf/ServerCachingService.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ message IndexMaintainer {
optional bytes indexWhere = 29;
repeated ColumnReference indexWhereColumns = 30;
optional bool isCDCIndex = 31;
optional int32 dataTableSaltBuckets = 32;
}

message TransformMaintainer {
Expand Down