[WIP] PHOENIX-7623: Add a profile which runs ITs with phoenix ha client - #2508
Open
Divneet18 wants to merge 1 commit into
Open
[WIP] PHOENIX-7623: Add a profile which runs ITs with phoenix ha client#2508Divneet18 wants to merge 1 commit into
Divneet18 wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Divneet18
changed the title
There was a problem hiding this comment.
Pull request overview
Adds an HA-enabled Maven test profile using loopback aliases to exercise HA client paths against one HBase cluster.
Changes:
- Adds single-cluster HA test infrastructure and lifecycle handling.
- Enables HA mode for parallel stats-disabled integration tests.
- Updates Cbrt integration tests and exposes supporting APIs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
phoenix-core/pom.xml |
Adds the ha-enabled profile. |
TestUtil.java |
Supplies HA test properties. |
BaseTest.java |
Adds HA cluster setup, URLs, and teardown. |
HighAvailabilityTestingUtility.java |
Implements single-cluster HA aliases. |
ParallelStatsDisabledIT.java |
Activates HA setup for subclasses. |
CbrtFunctionEnd2EndIT.java |
Passes HA connection properties. |
ReadOnlyProps.java |
Makes the empty constructor public. |
HighAvailabilityGroup.java |
Makes group closure public. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| private ReadOnlyProps() { | ||
| public ReadOnlyProps() { |
| public static void tearDownForHA() throws Exception { | ||
| if (Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))) { | ||
| try { | ||
| DriverManager.deregisterDriver(PhoenixDriver.INSTANCE); |
| * Someone calling close on this would make it unusable, since the state would become closed. | ||
| */ | ||
| void close() { | ||
| public void close() { |
Comment on lines
+53
to
+54
| System.out.println("get url " + getUrl()); | ||
| conn = DriverManager.getConnection(getUrl(), PropertiesUtil.deepCopy(TEST_PROPERTIES)); |
| import org.apache.phoenix.jdbc.HighAvailabilityGroup; | ||
| import org.apache.phoenix.jdbc.HighAvailabilityPolicy; | ||
| import org.apache.phoenix.jdbc.HighAvailabilityTestingUtility; | ||
| import static org.apache.phoenix.jdbc.HighAvailabilityTestingUtility.getHighAvailibilityGroup; |
| import org.apache.phoenix.jdbc.HighAvailabilityTestingUtility; | ||
| import static org.apache.phoenix.jdbc.HighAvailabilityTestingUtility.getHighAvailibilityGroup; | ||
| import org.apache.phoenix.jdbc.PhoenixConnection; | ||
| import org.apache.phoenix.jdbc.PhoenixMonitoredConnection; |
Comment on lines
+58
to
+61
| if (Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))) { | ||
| // Set up HA cluster in single-cluster mode | ||
| setUpTestClusterForHA(new ReadOnlyProps(props.entrySet().iterator()), | ||
| new ReadOnlyProps(props.entrySet().iterator())); |
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?
This PR adds a Maven profile (ha-enabled) that allows existing integration tests to run through Phoenix HA client code paths (FailoverPhoenixConnection, ParallelPhoenixConnection) using a single HBase cluster. When activated with mvn test
-Pha-enabled, tests exercise the HA connection infrastructure without spinning up two physical clusters. The approach uses loopback aliases (localhost vs 127.0.0.1) for the ACTIVE and STANDBY cluster URLs — both resolve to the same cluster but
are distinct strings, satisfying the HA URL validation requirement.
Why are the changes needed?
The original PR #2205 converted 300+ integration tests to HA mode but was closed because it spun up 2 complete HBase clusters for every test, which was too resource-intensive. This PR achieves the same HA code coverage (testing
FailoverPhoenixConnection wrappers, HAGroupStoreRecord creation, HA URL handling, connection pooling with HA wrappers) while using only 1 physical cluster — reducing resource usage by 50%. CbrtFunctionEnd2EndIT is converted as
proof-of-concept, with the pattern ready to apply to additional tests.
Does this PR introduce any user-facing change?
No. All changes are to test infrastructure only. No production behavior is changed. The two production code changes (making HighAvailabilityGroup.close() public and ReadOnlyProps() constructor public) only affect visibility for test access.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Yes. This patch was co-authored using Claude Code (Claude Opus 4.6/4.7). The AI assisted with investigating the root cause of connection timeout failures, identifying the loopback alias approach, implementing the fix, and debugging the
separate alias derivation for ZK and master configs.