lokiore opened a new pull request, #2580:
URL: https://github.com/apache/phoenix/pull/2580
### What changes were proposed in this pull request?
`ConnectionQueryServicesImpl.getMetaDataCache()` returned the `volatile
latestMetaData` field directly, with no null guard:
```java
public PMetaData getMetaDataCache() {
return latestMetaData;
}
```
This PR adds a guard so a closed connection surfaces a descriptive exception
instead of a bare NPE. The read is done once into a local so the null-check and
the returned reference are guaranteed to be the same value (no
time-of-check/time-of-use re-read window):
```java
public PMetaData getMetaDataCache() {
PMetaData cache = latestMetaData;
if (cache == null) {
throwConnectionClosedException();
}
return cache;
}
```
`throwConnectionClosedException()` is the same private helper that the
existing `throwConnectionClosedIfNullMetaData()` (used by `addTable`,
`removeTable`, `pruneTables`, the internal `getTable` path, etc.) delegates to
— it throws `IllegalStateException("Connection to the cluster is closed")`.
`getMetaDataCache()` was the one cache accessor missing this guard.
### Why are the changes needed?
`close()` sets `latestMetaData = null`. When a connection is closed
concurrently with query compilation (for example, connection-pool teardown or a
cluster role transition), an in-flight compile reaches the cache via
`PhoenixConnection.getTableRef()` / `getFunction()` / `getSchema()`, which call
`getMetaDataCache().getTableRef(key)` (and friends) on the `null` return. The
result is a **bare, causeless `NullPointerException`** with no message —
difficult to attribute to a closed connection during triage.
Every other accessor/mutator on this class already guards this state and
throws the descriptive `IllegalStateException`; `getMetaDataCache()` was
inconsistent. This change makes the failure mode uniform and diagnosable.
### Does this PR introduce _any_ user-facing change?
No behavioral change on an open connection: when `latestMetaData` is
present, the method returns exactly the same cache reference as before. The
only change is the failure mode when the connection has been closed — an opaque
`NullPointerException` is upgraded to a descriptive
`IllegalStateException("Connection to the cluster is closed")`. No caller
relies on a `null` return (every production caller immediately dereferences the
result), so no caller is regressed.
### How was this patch tested?
Added unit tests in `ConnectionQueryServicesImplTest` exercising a **real**
`ConnectionQueryServicesImpl` instance (not a mock, so the real guard executes):
- `testGetMetaDataCacheThrowsWhenClosed` — reflectively nulls
`latestMetaData` (closed-connection state) and asserts `getMetaDataCache()`
throws `IllegalStateException` with message `"Connection to the cluster is
closed"`.
- `testGetMetaDataCacheReturnsCacheWhenOpen` — asserts the method returns
the cache unchanged when present.
Both pass (`Tests run: 2, Failures: 0, Errors: 0`) via a reactor build (`mvn
-pl phoenix-core -am test`). `mvn spotless:apply` reports the tree clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8 (1M context))
--
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]
