lokiore opened a new pull request, #2516:
URL: https://github.com/apache/phoenix/pull/2516
<!--
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.
-->
Defensive null/empty check at the two production callsites of
`ConnectionQueryServicesImpl.getLiveRegionServers()`:
1.
`phoenix-core-client/.../util/GetClusterRoleRecordUtil.java::getClusterRoleRecord`
(~L92) — used by `HighAvailabilityGroup.getClusterRoleRecordFromEndpoint(...)`
on the CCF cluster-role fetch path.
2.
`phoenix-core-client/.../util/ValidateLastDDLTimestampUtil.java::validateLastDDLTimestamp`
(~L103) — used by query-path callers gated on
`LAST_DDL_TIMESTAMP_VALIDATION_ENABLED`.
If `getLiveRegionServers()` returns `null` or an empty list, refresh inline
via `refreshLiveRegionServers()` and re-fetch. If the list is still null/empty
after refresh, throw a descriptive `SQLException` so the caller sees a clear
error instead of a stack-walked `NullPointerException`.
The existing retry/recovery block at `GetClusterRoleRecordUtil.java:122-148`
is unchanged. It catches generic `Exception` and refreshes/recurses, and
continues to do so for genuine failures (auth, transport, RPC). What this PR
removes is its NPE-recovery responsibility: now there is no NPE to catch on the
first-call path, and the catch block only fires for real failures.
JIRA: https://issues.apache.org/jira/browse/PHOENIX-7765 (sub-task of
PHOENIX-7562)
### Why are the changes needed?
<!--
Please clarify why the changes are needed.
-->
`ConnectionQueryServicesImpl` declares `liveRegionServers` as a volatile
field with no initializer, defaulting to `null`. The list is auto-populated at
CQSI init only when `LAST_DDL_TIMESTAMP_VALIDATION_ENABLED` is true (default
`false`). On the default config path, the field stays null after init.
Both util callers (`getClusterRoleRecord`, `validateLastDDLTimestamp`) call
`getLiveRegionServers()` and immediately invoke
`regionServers.get(ThreadLocalRandom.current().nextInt(regionServers.size()))`
— `.size()` on a null reference produces a `NullPointerException` on first
invocation.
The existing catch block recovers transparently — it logs an `ERROR`,
refreshes the list, and recurses with `doRetry=false`. The recursive call sees
the now-populated list and returns successfully. So no exception escapes to the
caller; this is **not** a user-visible defect.
What is user-visible is the spurious ERROR log line per first invocation. On
a busy CCF cluster, every client connection's first cluster-role fetch logs:
```
ERROR util.GetClusterRoleRecordUtil(125): Error in getting ClusterRoleRecord
for <ha-group> from url <url>
java.lang.NullPointerException
at
org.apache.phoenix.util.GetClusterRoleRecordUtil.getClusterRoleRecord(GetClusterRoleRecordUtil.java:95)
...
```
These are not real errors — they are lazy-init noise. They flood operator
log surfaces, mask genuine errors, and create false-positive alerts in
log-based monitoring.
This PR addresses the lazy-init noise, not the (already-handled) NPE escape.
The framing is "remove spurious ERROR log churn on first cluster-role /
DDL-timestamp call," not "fix NPE."
### 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 (`PHOENIX-7562-feature-new`). The
change is operator-log-cleanup only; no behavioral or wire-format change. The
first-call path that previously logged ERROR + recovered now silently populates
the list and proceeds.
### 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.
-->
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, 14.863s
mvn spotless:apply -DskipTests -q
# silent success, zero drift
mvn -pl phoenix-core failsafe:integration-test \
-Dit.test=FailoverPhoenixConnection2IT -DfailIfNoTests=false
# Tests run: 13, Failures: 1, Errors: 0, Skipped: 1
# (1 pre-existing unrelated failure on
testAllWrappedConnectionsClosedAfterStandbyAndZKDownAsync —
# connection-cleanup timing in STANDBY+ZK-down test, unaffected by this
change)
```
**Empirical evidence — `LOGGER.error` count delta on
`FailoverPhoenixConnection2IT`:**
```
$ grep -c "Error in getting ClusterRoleRecord" failsafe-output.txt
# pre-fix: 36
# post-fix: 34
```
Verified all 34 remaining errors are genuine — searched the post-fix output
for stack frames containing `NullPointerException` co-occurring with `Error in
getting ClusterRoleRecord`:
```
$ grep -B1 "java.lang.NullPointerException" failsafe-output.txt \
| grep "Error in getting ClusterRoleRecord" | wc -l
# 0
```
The 34 remaining errors are from genuine ZK-down / failover /
connection-restart scenarios:
```
9 testConnectionWhenTwoZKRestarts
7 testFailoverCanFinishWhenOneZKDownWithCQS
7 testFailoverCanFinishWhenOneZKDown
5 testAllWrappedConnectionsClosedAfterStandbyAndZKDownAsync
3 testConnectionWhenStandbyZKRestarts
1 testStaleCrrVersionDetection
1 testAllWrappedConnectionsNotClosedAfterStandbyURLChange
1 testAllWrappedConnectionsClosedAfterActiveURLChange
```
**Caller sweep:** the only two production callers of
`getLiveRegionServers()` in `phoenix-core-client/` are the two files this PR
patches. Verified via:
```
$ git grep -n "getLiveRegionServers()" phoenix-core-client/src/main/java/
phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServices.java:203:
List<ServerName> getLiveRegionServers();
phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:5957:
return this.liveRegionServers;
phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionlessQueryServicesImpl.java:564:
...
phoenix-core-client/src/main/java/org/apache/phoenix/query/DelegateConnectionQueryServices.java:236:
...
phoenix-core-client/src/main/java/org/apache/phoenix/util/GetClusterRoleRecordUtil.java:96:
← patched
phoenix-core-client/src/main/java/org/apache/phoenix/util/ValidateLastDDLTimestampUtil.java:103:
← patched
```
### 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.
-->
This change was developed with assistance from an AI coding tool (Claude
Code, model Opus 4.7) per the [ASF Generative Tooling
policy](https://www.apache.org/legal/generative-tooling.html). All code was
reviewed by a human committer before commit; all tests were authored and
verified locally.
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]
