You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "lokiore (via GitHub)" <gi...@apache.org> on 2026/06/11 05:32:44 UTC
[PR] PHOENIX-7765 :- Eliminate spurious ERROR log on first cluster-role fetch [phoenix]
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7765 :- Eliminate spurious ERROR log on first cluster-role fetch [phoenix]
Posted by "lokiore (via GitHub)" <gi...@apache.org>.
lokiore merged PR #2516:
URL: https://github.com/apache/phoenix/pull/2516
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org