lokiore commented on code in PR #2611:
URL: https://github.com/apache/phoenix/pull/2611#discussion_r4010623408
##########
phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMaintainer.java:
##########
@@ -198,11 +196,9 @@ public static Iterator<PTable>
maintainedLocalOrGlobalIndexesWithoutMatchingStor
return Iterators.filter(indexes, new Predicate<PTable>() {
@Override
public boolean apply(PTable index) {
- return sendIndexMaintainer(index) && ((index.getIndexType() ==
IndexType.GLOBAL
+ return sendIndexMaintainer(index) && ((IndexUtil.isGlobalIndex(index)
&& (dataTable.getImmutableStorageScheme() !=
index.getImmutableStorageScheme()
- || connection.getQueryServices().getConfiguration().getBoolean(
- SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
- DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED)))
+ ||
IndexUtil.isServerSideImmutableIndexMaintenanceEnabled(dataTable, connection)))
Review Comment:
Confirmed — the analysis is correct. The storage-scheme-mismatch term is the
left operand of the inner `||`, so for a mismatched-scheme index the predicate
short-circuits to `true` there and the `ROW_TIMESTAMP` carve-out on the right
operand (`isServerSideImmutableIndexMaintenanceEnabled`) is never consulted.
There is also no client-side rescue: `getClientMaintainedIndexes` routes an
immutable `ROW_TIMESTAMP` table through
`maintainedGlobalIndexesWithMatchingStorageScheme`, which excludes a
cross-scheme index, so it cannot safely be client-maintained without a deeper
change.
For a covered global index this routing is unchanged by this PR — the
mismatch term is pre-existing (relative to the merge-base, the only edits to
that operand are `IndexType.GLOBAL` -> `IndexUtil.isGlobalIndex` and the direct
flag read -> the helper). So a covered mismatched-scheme index on a
`ROW_TIMESTAMP` immutable table was already server-maintained and re-stamped
before this flip. The `isGlobalIndex` broadening does newly bring
`UNCOVERED_GLOBAL` mismatched-scheme indexes onto the server path, extending
the same corner to the uncovered variant.
The correct fix is a server-side `ROW_TIMESTAMP` exemption in the re-stamp
path (`setTimestamps`) rather than routing changes; it is orthogonal to this
default flip and best handled separately.
--
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]
