Add integration test for rebalance in upsert tables #11568

KKcorps · 2023-09-11T19:51:23Z

Mostly tests that partitions move as a whole unit to new servers

codecov-commenter · 2023-09-12T07:06:32Z

Codecov Report

Merging #11568 (e0c8532) into master (24dba83) will decrease coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #11568      +/-   ##
============================================
- Coverage     63.10%   63.08%   -0.02%     
  Complexity     1118     1118              
============================================
  Files          2342     2342              
  Lines        125916   125917       +1     
  Branches      19370    19370              
============================================
- Hits          79454    79440      -14     
- Misses        40813    40825      +12     
- Partials       5649     5652       +3

Flag	Coverage Δ
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`63.06% <0.00%> (-0.01%)`	⬇️
java-17	`62.95% <0.00%> (+<0.01%)`	⬆️
java-20	`62.94% <0.00%> (-0.02%)`	⬇️
temurin	`63.08% <0.00%> (-0.02%)`	⬇️
unittests	`63.08% <0.00%> (-0.02%)`	⬇️
unittests1	`67.23% <0.00%> (-0.01%)`	⬇️
unittests2	`14.42% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...spi/utils/builder/ControllerRequestURLBuilder.java	`0.00% <0.00%> (ø)`
...inot/controller/helix/ControllerRequestClient.java	`37.34% <0.00%> (ø)`

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

...ts/src/test/java/org/apache/pinot/integration/tests/UpsertTableRebalanceIntegrationTest.java

Jackie-Jiang · 2023-09-15T02:12:28Z

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

+  @Override
+  protected void waitForAllDocsLoaded(long timeoutMs)
+      throws Exception {
+    Thread.sleep(5000L); //TODO: Wait for some time for rebalance to kickoff


We want to avoid sleeping a fixed amount of time. For fast machine, it can waste time; for slow machine, it can cause flakiness. Any specific reason why waiting for condition itself is not enough?

Yeah, waiting on condition itself triggers an early termination since the rebalance simply hasn't started and all segments are still available.

npawar · 2023-09-19T22:41:39Z

Mostly tests that partitions move as a whole unit to new servers

is there an jira that prompted this item? did this not work in some env / scenario?

KKcorps · 2023-09-20T11:35:56Z

Mostly tests that partitions move as a whole unit to new servers

is there an jira that prompted this item? did this not work in some env / scenario?

Yes, there are cases when ideal state diverges from instance partition map and it created prod issues.

@klsince is working on the fix for the issue.

#11628

Jackie-Jiang · 2023-09-21T02:15:26Z

@klsince Can you help review this test?

klsince · 2023-09-21T23:18:08Z

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

+    return 600;
+  }
+
+  protected long getCurrentCountStarResultWithoutNulls(String tableName, Schema schema) {


curious why need to check on NULL rows?

aah because we have observed an issue in partial upsert tables in the past where some columns always contain null even though the correct data is coming in the stream.

klsince · 2023-09-21T23:23:35Z

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

+    // Check the number of replicas after rebalancing
+    finalReplicas = _resourceManager.getServerInstancesForTable(getTableName(), TableType.REALTIME).size();
+
+    // Check that a replica has been added


I assume you meant a server has been added? as from the comment below, rf=1

klsince · 2023-09-21T23:28:27Z

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

+    waitForAllDocsLoaded(600_000L);
+
+    // number of instances assigned can't be more than number of partitions for rf = 1
+    verifySegmentAssignment(rebalanceResult.getSegmentAssignment(), 5, getNumKafkaPartitions());


iiuc, I assume the number of kafka partitions is 3? then, it may be interesting to test by adding one more server (4 servers in total) to check that one server is left idle w/o taking any table partition.

No the default is 2 partitons and that scenario is already covered.

klsince · 2023-09-21T23:35:12Z

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

+
+import static org.testng.Assert.assertEquals;
+
+public class PartialUpsertTableRebalanceIntegrationTest extends BaseClusterIntegrationTest {


looks like the test cases are not specific to partial upsert? or I must have missed some details 🤔

Yeah it covers both upsert and partial upsert. I have added the table config to be partial upsert cause we observed weird scenarios there sometimes where some columns are always null.

klsince

LGTM
just two non-blocking comments

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java

KKcorps changed the title ~~Add integration test for rebalance in upsert tables~~ WIP: Add integration test for rebalance in upsert tables Sep 12, 2023

KKcorps force-pushed the upsert_rebalance_tes branch 3 times, most recently from 088a2b2 to eba73c6 Compare September 13, 2023 12:03

Jackie-Jiang added the testing label Sep 14, 2023

KKcorps changed the title ~~WIP: Add integration test for rebalance in upsert tables~~ Add integration test for rebalance in upsert tables Sep 14, 2023

Jackie-Jiang reviewed Sep 15, 2023

View reviewed changes

klsince reviewed Sep 21, 2023

View reviewed changes

KKcorps force-pushed the upsert_rebalance_tes branch from 638a567 to 905af70 Compare October 5, 2023 08:12

KKcorps requested a review from klsince October 5, 2023 08:22

klsince approved these changes Oct 6, 2023

View reviewed changes

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java Outdated Show resolved Hide resolved

...test/java/org/apache/pinot/integration/tests/PartialUpsertTableRebalanceIntegrationTest.java Outdated Show resolved Hide resolved

KKcorps and others added 9 commits October 9, 2023 09:37

Add integration test for rebalance in upsert tables

5a9cfb4

Add reload test as well

b2698bb

Reload tests

31ce411

Remove upsert integration test since partial upsert tests cover both

56f916c

Add explicit status checks for rebalance/reload jobs rather than sleep

9e443e9

Fix flakiness due to segment commit during reload

6865515

Use new RebalanceConfig class

ff3fa5e

Add missing partial upsert cases

937b1e2

Refactor jobstatus to segment reload status

e0c8532

KKcorps force-pushed the upsert_rebalance_tes branch from adf9e77 to e0c8532 Compare October 9, 2023 04:07

KKcorps merged commit 52d16f7 into apache:master Oct 9, 2023
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration test for rebalance in upsert tables #11568

Add integration test for rebalance in upsert tables #11568

KKcorps commented Sep 11, 2023

codecov-commenter commented Sep 12, 2023 •

edited

Loading

Jackie-Jiang Sep 15, 2023

KKcorps Sep 18, 2023

npawar commented Sep 19, 2023

KKcorps commented Sep 20, 2023 •

edited

Loading

Jackie-Jiang commented Sep 21, 2023

klsince Sep 21, 2023

KKcorps Sep 24, 2023

klsince Sep 21, 2023

KKcorps Sep 24, 2023

klsince Sep 21, 2023

KKcorps Sep 24, 2023

klsince Sep 21, 2023

KKcorps Sep 24, 2023

klsince left a comment


		import static org.testng.Assert.assertEquals;

		public class PartialUpsertTableRebalanceIntegrationTest extends BaseClusterIntegrationTest {

Add integration test for rebalance in upsert tables #11568

Add integration test for rebalance in upsert tables #11568

Conversation

KKcorps commented Sep 11, 2023

codecov-commenter commented Sep 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npawar commented Sep 19, 2023

KKcorps commented Sep 20, 2023 • edited Loading

Jackie-Jiang commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klsince left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 12, 2023 •

edited

Loading

KKcorps commented Sep 20, 2023 •

edited

Loading