Skip to content

PHOENIX-7964: Wait for Phoenix system tables to initialize before sta… - #14

Open
jinggou wants to merge 5 commits into
apache:mainfrom
jinggou:PHOENIX-7964
Open

PHOENIX-7964: Wait for Phoenix system tables to initialize before sta…#14
jinggou wants to merge 5 commits into
apache:mainfrom
jinggou:PHOENIX-7964

Conversation

@jinggou

@jinggou jinggou commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

…rting REST server

Before starting the REST server in integration tests, ensure Phoenix system tables
are fully initialized by calling TestUtils.awaitPhoenixReady().

This eliminates the race condition that caused intermittent test failures with:
org.apache.phoenix.exception.PhoenixIOException: SYSTEM.CATALOG
Caused by: org.apache.hadoop.hbase.TableNotFoundException: SYSTEM.CATALOG

The fix adds TestUtils.awaitPhoenixReady(url) calls to all 82 integration test classes, either directly in their @BeforeClass methods or through inheritance from base classes like BaseSegmentScanIT, KclTestBase, UpdateItemBaseTests, and SegmentScanIT.

The awaitPhoenixReady() method polls for up to 30 seconds (vs RESTServer's 5 seconds) for Phoenix to become ready, providing sufficient time for SYSTEM.CATALOG initialization even on slow CI workers.

Changes:

  • Added TestUtils.awaitPhoenixReady() calls to 58 integration test classes
  • Fixed DeleteTableIT to construct Phoenix JDBC URL before REST server initialization
  • 24 additional test classes inherit the fix through base class inheritance
  • Total coverage: 82/82 integration test classes

…rting REST server

Before starting the REST server in integration tests, ensure Phoenix system tables
 are fully initialized by calling TestUtils.awaitPhoenixReady().

This eliminates the race condition that caused intermittent test failures with:
  org.apache.phoenix.exception.PhoenixIOException: SYSTEM.CATALOG
  Caused by: org.apache.hadoop.hbase.TableNotFoundException: SYSTEM.CATALOG

The fix adds TestUtils.awaitPhoenixReady(url) calls to all 82 integration test classes,
either directly in their @BeforeClass methods or through inheritance from base classes
like BaseSegmentScanIT, KclTestBase, UpdateItemBaseTests, and SegmentScanIT.

The awaitPhoenixReady() method polls for up to 30 seconds (vs RESTServer's 5 seconds)
for Phoenix to become ready, providing sufficient time for SYSTEM.CATALOG initialization
even on slow CI workers.

Changes:
- Added TestUtils.awaitPhoenixReady() calls to 58 integration test classes
- Fixed DeleteTableIT to construct Phoenix JDBC URL before REST server initialization
- 24 additional test classes inherit the fix through base class inheritance
- Total coverage: 82/82 integration test classes
restServer = new RESTServer(utility.getConfiguration());
restServer.run();

String url = "http://" + restServer.getServerAddress();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to build a JDBC URL from the ZK quorum here, not the rest server url.

@palashc palashc added the tests Change in Test Classes only. label Jul 23, 2026
Comment on lines +65 to +66
TestUtils.awaitPhoenixReady(url);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 2 options:

  1. remove awaitPhoenixReady() from this test
  2. remove this test entirely

I prefer option 1, which means this single IT test will not be protected by awaitPhoenixReady() and can lead to flaky test failure, which is okay.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, thank you

utility = new HBaseTestingUtility(conf);
setUpConfigForMiniCluster(conf);
utility.startMiniCluster();
String zkQuorum = "127.0.0.1:" + utility.getZkCluster().getClientPort();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we have 127.0.0.1 here instead of localhost (everywhere else)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!

@virajjasani virajjasani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to consider before we are good to go

setUpConfigForMiniCluster(conf);
utility.startMiniCluster();
String zkQuorum = "127.0.0.1:" + utility.getZkCluster().getClientPort();
String zkQuorum = "localhost:" + utility.getZkCluster().getClientPort();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should let it be 127.0.0.1 otherwise we have test failure in the latest build?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically no change needed in this IT test :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Change in Test Classes only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants