HIVE-29802: Handle ClusterNotReadyException in ProactiveEviction and add Kerberos-aware tests - #6677
Open
g3rg0 wants to merge 3 commits into
Open
HIVE-29802: Handle ClusterNotReadyException in ProactiveEviction and add Kerberos-aware tests#6677g3rg0 wants to merge 3 commits into
g3rg0 wants to merge 3 commits into
Conversation
…add Kerberos-aware tests When no LLAP daemons have started, the ZK paths don't exist yet. The PathChildrenCache attempts to create them using CREATOR_ALL_ACL, which requires an authenticated identity. Since HS2 is unauthenticated, ZooKeeper rejects this with InvalidACLException, surfacing as ClusterNotReadyException. Previously, this exception propagated as a RuntimeException from evict(), causing DDL operations like DROP DATABASE to fail when LLAP hadn't started. Fix: catch ClusterNotReadyException in ProactiveEviction.evict() and return silently — if no daemons are registered, there's nothing cached to evict. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR addresses failures in Kerberos-enabled environments when LLAP hasn’t registered in ZooKeeper yet by introducing a dedicated ClusterNotReadyException, handling it gracefully in proactive eviction, and adding regression tests for both the retry and end-to-end eviction paths.
Changes:
- Introduce
ClusterNotReadyExceptionand throw it fromZkRegistryBase.ensureInstancesCache()when startup cannot proceed due to ACL/auth conditions. - Catch
ClusterNotReadyExceptioninProactiveEviction.evict()to skip eviction instead of failing DDLs. - Add unit/integration tests covering InvalidACL retry behavior and Kerberos-like end-to-end proactive eviction behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ql/src/test/org/apache/hadoop/hive/llap/TestProactiveEviction.java | Adds embedded-ZK / Kerberos-mocked end-to-end proactive eviction tests. |
| ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java | Swallows ClusterNotReadyException to avoid failing eviction callers when cluster isn’t ready. |
| llap-client/src/test/org/apache/hadoop/hive/llap/registry/impl/TestLlapZookeeperRegistryImpl.java | Adds tests for InvalidACL retry and ClusterNotReadyException behavior. |
| llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java | Throws ClusterNotReadyException from PathChildrenCache startup failure paths. |
| llap-client/src/java/org/apache/hadoop/hive/registry/ClusterNotReadyException.java | Adds new exception type used to signal “cluster not ready” conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Restore thread interrupt flag with Thread.currentThread().interrupt() when catching InterruptedException in ZkRegistryBase, so higher-level cancellation/shutdown logic can observe the interrupt. - Include the exception in the LOG.error call so the cause is not lost. - Remove redundant TestingServer.start() call since the default constructor auto-starts the server. - Make test cleanup robust: close curatorFramework and TestingServer via CloseableUtils.closeQuietly() in tearDown with null guards, ensuring resources are released even if setup partially fails or a test throws. - Wrap CuratorFramework usage in TestLlapZookeeperRegistryImpl tests with try/finally to close the client and prevent thread/socket leaks. - Relax Mockito verification from atLeast(4) to atLeast(2) since the exact call count varies across Curator/ZK versions. - Rename testClusterNotReadyExceptionIsThrownWhenZkNodeNotExists to testClusterNotReadyExceptionOnImmediateTimeoutWithSecureAcl to accurately reflect that it tests the immediate-failure path (timeout=0). - Add testClusterNotReadyExceptionAfterRetriesWithSecureAcl which uses a small positive timeout (100ms) and asserts that retries occur before the deadline is reached, covering the retry-until-exhausted path. - Add serialVersionUID to ClusterNotReadyException and additional constructors accepting message and message+cause for more descriptive upstream logging without relying solely on nested exception text. - Add assertion in testEvictWithKerberosAndRegisteredComputes verifying that 2 LLAP instances are discovered before eviction is triggered. - Use static imports for Mockito.mock, when, verify, atLeast, any in TestLlapZookeeperRegistryImpl for cleaner test code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@g3rg0 : tests passed, patch overall looks good to me |
Contributor
Author
@abstractdog Sure, I'm working on it.
|
- Remove unnecessary throws IOException from tearDown since CloseableUtils.closeQuietly() does not throw checked exceptions. - Use static imports for Mockito.mock, mockStatic, and when for cleaner test code consistent with TestLlapZookeeperRegistryImpl. - Organize imports: java.* first, then third-party/project imports, then static imports grouped by package. - Add explicit assertion to testEvictWithKerberosWithoutComputeInstances: wrap evict() in try/catch and fail if any exception is thrown, making the "handles gracefully" contract explicit rather than implicit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
abstractdog
self-requested a review
August 6, 2026 15:01
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Why are the changes needed?
In Kerberos-enabled deployments, HS2 connects to ZooKeeper without SASL authentication (it only needs read access to discover LLAP daemons). When no LLAP daemons have started, the ZK paths (e.g. /llap-sasl/user-hive) don't exist yet. The PathChildrenCache attempts to create them with CREATOR_ALL_ACL, which ZooKeeper rejects with InvalidACLException since the client has no authenticated identity.
This surfaced as an unhandled RuntimeException from ProactiveEviction.evict(), causing DDL operations like DROP DATABASE to fail when LLAP hadn't started.
Does this PR introduce any user-facing change?
No.
How was this patch tested?