lokiore opened a new pull request, #2515:
URL: https://github.com/apache/phoenix/pull/2515
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
https://phoenix.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]
PHOENIX-XXXX Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes, and
prefix it with the JIRA id, e.g. 'PHOENIX-XXXX Your PR title ...'.
6. If possible, provide a concise example to reproduce the issue for a
faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section
is to outline the changes and how this PR fixes the issue.
-->
Three localized changes in `phoenix-core-client` that propagate the HA group
through cloned `PhoenixConnection`s, plus an integration test that locks the
regression:
1. **`PhoenixConnection.java` — copy-constructor `haGroup` propagation.**
The four copy constructors that take an existing `PhoenixConnection` as the
first argument all delegate to the 9-arg private constructor with `null` passed
for the cloned connection's `haGroup` field. Replaced with `connection.haGroup`
so cloned connections inherit the parent's HA group. Affected ctors:
`(PhoenixConnection, boolean, boolean)`, `(PhoenixConnection, MutationState)`,
`(PhoenixConnection, Properties)`, and `(PhoenixConnection,
ConnectionQueryServices, Properties)`. The 1-arg `(PhoenixConnection)` and the
`(PhoenixConnection, long scn)` ctors delegate transitively and inherit the fix.
2. **`PhoenixConnection.java` — Javadoc disambiguation on the two HA
fields.** The `haGroupName` (`String`) and `haGroup` (`HighAvailabilityGroup`)
fields share an identical `// Only available for connection which is part of HA
Connections` comment. Replaced with field-specific descriptions: `haGroupName`
is the wire-tag string used for the `_HAGroupName` HBase attribute and is set
on every connection that resolved through an HA route (including cloned
connections); `haGroup` is the resolved group object used for failover
orchestration and cluster-role lookups, set on root HA connections and
inherited by clones via the copy constructors.
3. **`ConnectionQueryServices.java` — interface ergonomics.** The 3-arg
`connect(String url, Properties info, HighAvailabilityGroup haGroup)` overload
is currently abstract. Convert to `default` delegating to the 2-arg
`connect(url, info)`. The three internal implementors
(`ConnectionQueryServicesImpl`, `ConnectionlessQueryServicesImpl`,
`DelegateConnectionQueryServices`) already override the 3-arg form with a
`haGroup`-using body. The default is for source-compatibility with any
3rd-party `ConnectionQueryServices` implementor that has not yet adopted the
3-arg overload — those implementors get the pre-existing 2-arg behavior
(haGroup discarded) instead of a `NoSuchMethodError`.
`ReplicationLogGroupIT.java` — adds
`testUpsertSelectReplicatesViaCloneConnection` to lock the regression. The test
exercises an `UPSERT SELECT` of 250 rows (above the default `MUTATE_BATCH_SIZE
= 100`) so the in-loop flush at `UpsertCompiler.upsertSelect` fires through the
cloned connection's `MutationState.send()` path — the only path where the
missing `haGroup` field is observable.
JIRA: https://issues.apache.org/jira/browse/PHOENIX-7898
### Why are the changes needed?
<!--
Please clarify why the changes are needed.
-->
Cloned `PhoenixConnection`s are produced by
`MutatingParallelIteratorFactory.newIterator(...)` (the server-side coprocessor
path for `UPSERT SELECT` and `DELETE`), which calls `new
PhoenixConnection(this.connection)` per parallel iterator scope. Because each
clone receives `haGroup = null`, the guard inside
`MutationState.annotateMutationWithMetadata`:
```java
if (connection.getHAGroup() != null &&
StringUtils.isNotBlank(connection.getHAGroupName())) {
haGroupName = Bytes.toBytes(connection.getHAGroupName());
}
```
evaluates `false` on every mutation flushed through the cloned connection.
The `_HAGroupName` HBase attribute therefore is **not** attached to those
mutations, and the consistent_failover (CCF) server's mutation-block contract —
which gates ACTIVE→STANDBY transitions on the presence of that attribute — is
not enforced for the affected write paths.
Threshold sensitivity: with `MUTATE_BATCH_SIZE = 100` (default), an `UPSERT
SELECT` of N rows triggers `floor(N/100)` in-loop flushes through the cloned
connection's `MutationState.send()` path. The trailing `N mod 100` rows are
joined back to the parent connection and committed via the parent's path
(parent's `haGroup` is non-null), so those rows DO get tagged. For N=250: 200
mutations dropped (no `_HAGroupName`), 50 mutations tagged.
This is a correctness gap on the `PHOENIX-7562-feature-new` line and must be
fixed before that line merges to master.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as
new features, bug fixes, or other behavior changes.
-->
No
This is a pre-launch CCF feature branch. No customer-visible behavior
change; the fix corrects an internal contract that has not shipped to users.
The interface change converts an abstract method to a `default`, which is
source-compatible.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some
test cases that check the changes thoroughly including negative and positive
cases if possible.
-->
New integration test method `testUpsertSelectReplicatesViaCloneConnection`
in `ReplicationLogGroupIT`.
Local commands run on `PHOENIX-7562-feature-new` HEAD `8f9655a69e`:
```
mvn install -pl phoenix-core-client,phoenix-core,phoenix-core-server -am
-DskipTests
# BUILD SUCCESS, 5 reactor modules
mvn spotless:apply -DskipTests -q
# silent success — zero drift
mvn -pl phoenix-core failsafe:integration-test failsafe:verify \
-Dit.test='ReplicationLogGroupIT#testUpsertSelectReplicatesViaCloneConnection' \
-DfailIfNoTests=false
# pre-fix: Tests run: 1, Failures: 1 — assertion: target table
expected:<250> but was:<50>
# post-fix: Tests run: 1, Failures: 0 — target table = 250
```
Empirical pre/post-fix `dumpTableLogCount` on the same test:
```
pre-fix: T_source = 250 | T_target = 50 ← 200 dropped via 2x in-loop
flush through cloned conn (no _HAGroupName)
post-fix: T_source = 250 | T_target = 250 ← cloned conn inherits haGroup,
_HAGroupName tagged
```
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this
patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
Please refer to the [ASF Generative Tooling
Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Generated-by: Claude Code (Opus 4.7)
--
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]
