Validate Table Replication During Create/Update #13951

suddendust · 2024-09-07T13:24:09Z

Recently, we've observed that RT ingestion stopped simply because of not enough servers to fulfil the replication configured and no warning / error given to user. This PR adds a check for that during table update. For table create, we already do this check and don't let the user create the table if it fails.

…st all replicas

…lidity

codecov-commenter · 2024-09-07T14:17:41Z

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.39%. Comparing base (59551e4) to head (5b71dca).
Report is 1732 commits behind head on master.

Files with missing lines	Patch %	Lines
...not/common/assignment/InstancePartitionsUtils.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13951      +/-   ##
============================================
+ Coverage     61.75%   63.39%   +1.64%     
- Complexity      207     1483    +1276     
============================================
  Files          2436     2744     +308     
  Lines        133233   154069   +20836     
  Branches      20636    23779    +3143     
============================================
+ Hits          82274    97671   +15397     
- Misses        44911    49021    +4110     
- Partials       6048     7377    +1329

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.36% <94.44%> (+1.65%)`	⬆️
java-21	`63.29% <94.44%> (+1.66%)`	⬆️
skip-bytebuffers-false	`63.39% <94.44%> (+1.64%)`	⬆️
skip-bytebuffers-true	`63.26% <94.44%> (+35.54%)`	⬆️
temurin	`63.39% <94.44%> (+1.64%)`	⬆️
unittests	`63.39% <94.44%> (+1.64%)`	⬆️
unittests1	`56.06% <0.00%> (+9.17%)`	⬆️
unittests2	`33.87% <94.44%> (+6.14%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ege-st · 2024-09-09T14:12:40Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+    if (tagOverrideConfig != null && tagOverrideConfig.getRealtimeConsuming() != null
+        && tagOverrideConfig.getRealtimeCompleted() != null) {


Could you add a comment explaining the significance of these 3 references not being null?

You might want to use TagNameUtils? I think we allow having only one of CONSUMING/COMPLETED not null

ege-st · 2024-09-09T14:14:13Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+    if (consumingServersCnt < replication || completedServersCnt < replication) {
+      if (consumingServersCnt < replication && completedServersCnt < replication) {


OOC, what's the Consuming tag and Completed tag mean and how do they get assigned to servers?

Jackie-Jiang

It is more complicated when instanceAssignmentConfigMap is configured. We probably should do a dry run on instance assignment to verify if the config is valid

suddendust · 2024-09-15T17:10:56Z

@Jackie-Jiang Right. But can we do a dry run when the table doesn't even exist? The idea is to capture this before the table is even created.

Jackie-Jiang · 2024-09-18T20:35:22Z

We can trigger the same piece of code. It should throw exception when there are not enough servers to host the table

…tAllReplicas

Jackie-Jiang

We should be able to validate table config even when table doesn't exist. I think only validating instance assignment should be good enough. Do you see a requirement of validating table rebalance?

Jackie-Jiang · 2025-02-11T17:25:18Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+      LOGGER.error("Could not calculate target assignment for table: {} for the provided table config",
+          tableNameWithType);
+      throw e;


Let's wrap the error message into a new exception and not adding this error log

…tAllReplicas

suddendust · 2025-02-15T04:47:12Z

@Jackie-Jiang Made the changes. Summarising the existing and new behaviour here:

Existing Behaviour
New Table
OFFLINE: Invalid replication goes through
REALTIME: Invalid replication does not go through

Update Table
OFFLINE: Invalid replication goes through
REALTIME: Invalid replication goes through

New Behaviour
New Table
OFFLINE: Invalid replication does not go through.
REALTIME: Invalid replication does not go through.

Update Table
OFFLINE: Invalid replication does not go through.
REALTIME: Invalid replication does not go through.

Setting reassignInstances to true also catches invalid settings for replica-grouper based assignments.

Jackie-Jiang

Nice approach!

Jackie-Jiang · 2025-02-17T19:42:51Z

pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitionsUtils.java

@@ -137,7 +137,7 @@ public static InstancePartitions computeDefaultInstancePartitions(HelixManager h
        throw new IllegalStateException();
    }
    return computeDefaultInstancePartitionsForTag(helixManager, tableConfig.getTableName(),
-        instancePartitionsType.toString(), serverTag);
+        instancePartitionsType.toString(), serverTag, tableConfig.getReplication());


Let's directly pass in tableConfig

Jackie-Jiang · 2025-02-17T19:46:14Z

pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitionsUtils.java

@@ -161,6 +161,24 @@ public static InstancePartitions computeDefaultInstancePartitionsForTag(HelixMan
    return instancePartitions;
  }

+  public static InstancePartitions computeDefaultInstancePartitionsForTag(HelixManager helixManager,


We can replace the old method. I checked all the usage, and it is safe to use the new method in TierConfigUtils.

@klsince TierConfigUtils.getTieredInstancePartitionsForSegment() shouldn't always use default instance partitions, it should use the tier instance partitions if exists. Can you please take a look?

Jackie-Jiang · 2025-02-17T19:47:04Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+    try {
+      tableRebalancer.getInstancePartitionsMap(tableConfig, true, true, true);
+    } catch (Exception e) {
+      LOGGER.error("Exception calculating instance partitions for table: {}", tableConfig.getTableName());


Since we are throwing the exception out, no need to log the error here.

Jackie-Jiang · 2025-02-17T19:48:30Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+      tableRebalancer.getInstancePartitionsMap(tableConfig, true, true, true);
+    } catch (Exception e) {
+      LOGGER.error("Exception calculating instance partitions for table: {}", tableConfig.getTableName());
+      throw new RuntimeException("Exception calculating instance partitions for table:" + tableConfig.getTableName());


Add a space after :

Jackie-Jiang · 2025-02-17T19:49:20Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

+      throw new RuntimeException("Exception calculating instance partitions for table:" + tableConfig.getTableName());
+    }
+  }
+


(nit) Remove empty line

Jackie-Jiang · 2025-02-17T19:49:24Z

...-controller/src/test/java/org/apache/pinot/controller/api/PinotTableRestletResourceTest.java

+        instanceReplicaGroupPartitionConfig,
+        InstanceAssignmentConfig.PartitionSelector.FD_AWARE_INSTANCE_PARTITION_SELECTOR.name(), false);
+  }
+


(nit) Remove empty line

suddendust added 4 commits September 7, 2024 18:22

Validate during table create/update if sufficient servers exist to ho…

16aae22

…st all replicas

Handle tag overrides correctly: Use servers with tags to determine va…

58da6c7

…lidity

Call the validation method

0dfe4d5

Refactor

a0cfcff

Fix failing test case

a55fc6a

ege-st reviewed Sep 9, 2024

View reviewed changes

Jackie-Jiang added the user-experience Related to user experience label Sep 9, 2024

Jackie-Jiang reviewed Sep 9, 2024

View reviewed changes

WIP

4062a04

suddendust changed the title ~~Validate if Table Replication Factor Can be Honoured during Table Create/Update~~ [WIP] Validate if Table Replication Factor Can be Honoured during Table Create/Update Nov 1, 2024

suddendust added 8 commits February 10, 2025 20:20

WIP

4e4d4b4

WIP

6cd8df7

WIP

3b9ba46

WIP

bc38d61

WIP

86b3785

Merge branch 'master' of github.com:apache/pinot into validate_canHos…

fe14f17

…tAllReplicas

WIP

9c41f4f

Refactor test case

a0a4851

suddendust changed the title ~~[WIP] Validate if Table Replication Factor Can be Honoured during Table Create/Update~~ [WIP] Validate if Table Replication Factor Can be Honoured during Table Update Feb 11, 2025

suddendust added 2 commits February 11, 2025 22:07

Refactor

6741967

Refactor

6c3f5fa

Jackie-Jiang reviewed Feb 11, 2025

View reviewed changes

suddendust added 5 commits February 13, 2025 11:54

WIP

b71ad34

WIP

4c58b09

WIP

b46c474

Rollback inadvertent changes

c40309d

Rollback inadvertent changes

eeb3e05

TableRebalancer changes

1406547

suddendust changed the title ~~[WIP] Validate if Table Replication Factor Can be Honoured during Table Update~~ Validate Table Replication During Create/Update Feb 14, 2025

suddendust added 2 commits February 14, 2025 15:59

WIP

4760031

Merge branch 'master' of github.com:apache/pinot into validate_canHos…

e9d6c42

…tAllReplicas

suddendust added 4 commits February 15, 2025 10:47

Refactor test cases

9ddbab8

Refactor test cases

422b873

WIP

2c70d0e

Wrap exception

dc1cd3c

Jackie-Jiang reviewed Feb 17, 2025

View reviewed changes

suddendust and others added 9 commits February 18, 2025 15:23

Address comments and fix failing test case

9ecb89c

Add reason to exception message

61de9e0

Remove extra line

7ccd485

Refactor test case

f0156c1

Checked-in missed files

bb882da

Pass in table config directly

274f1c9

Remove unused method

1c4bd19

Docs

e7026fe

Cleanup

1f350e3

Jackie-Jiang approved these changes Feb 18, 2025

View reviewed changes

Fix test

5b71dca

Jackie-Jiang merged commit d76fe22 into apache:master Feb 18, 2025
21 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Table Replication During Create/Update #13951

Validate Table Replication During Create/Update #13951

suddendust commented Sep 7, 2024 •

edited

Loading

codecov-commenter commented Sep 7, 2024 •

edited

Loading

ege-st Sep 9, 2024

Jackie-Jiang Sep 9, 2024

ege-st Sep 9, 2024

Jackie-Jiang left a comment

suddendust commented Sep 15, 2024

Jackie-Jiang commented Sep 18, 2024

Jackie-Jiang left a comment •

edited

Loading

Jackie-Jiang Feb 11, 2025

suddendust commented Feb 15, 2025 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Feb 17, 2025

suddendust Feb 18, 2025

Jackie-Jiang Feb 17, 2025

suddendust Feb 18, 2025

Jackie-Jiang Feb 17, 2025

Jackie-Jiang Feb 17, 2025

Jackie-Jiang Feb 17, 2025

Jackie-Jiang Feb 17, 2025

suddendust Feb 18, 2025

		if (tagOverrideConfig != null && tagOverrideConfig.getRealtimeConsuming() != null
		&& tagOverrideConfig.getRealtimeCompleted() != null) {

		if (consumingServersCnt < replication \|\| completedServersCnt < replication) {
		if (consumingServersCnt < replication && completedServersCnt < replication) {

Validate Table Replication During Create/Update #13951

Validate Table Replication During Create/Update #13951

Conversation

suddendust commented Sep 7, 2024 • edited Loading

codecov-commenter commented Sep 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

suddendust commented Sep 15, 2024

Jackie-Jiang commented Sep 18, 2024

Jackie-Jiang left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddendust commented Feb 15, 2025 • edited Loading

Jackie-Jiang left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddendust commented Sep 7, 2024 •

edited

Loading

codecov-commenter commented Sep 7, 2024 •

edited

Loading

Jackie-Jiang left a comment •

edited

Loading

suddendust commented Feb 15, 2025 •

edited

Loading