Fix the replication in segment assignment strategy #9816

GSharayu · 2022-11-16T20:38:30Z

The Balanced and Replica group segment assignment strategy would assume replication to be fetched from only table config as tableConfig.getValidationConfig().getReplicationNumber() irrespective of whether its a realtime table or offline table
But for real time tables, if we have tableConfig.getValidationConfig().getReplicasPerPartitionNumber(), this should be preferred over table replication

No additional changes are required
Reference Issue: #9309
Also, issue #8804

GSharayu · 2022-11-16T20:39:07Z

@mcvsubbu @sajjad-moradi @jugomezv @snleee : please review

snleee

@GSharayu Do we have a case where we pick up BalancedNumSegmentAssignment for realtime table? I thought that we haven't wired the segment assignment strategy for realtime tables yet.

snleee · 2022-11-16T20:57:57Z

.../controller/helix/core/assignment/segment/strategy/BalancedNumSegmentAssignmentStrategy.java

@@ -51,6 +51,10 @@ public void init(HelixManager helixManager, TableConfig tableConfig) {
    SegmentsValidationAndRetentionConfig validationAndRetentionConfig = tableConfig.getValidationConfig();
    Preconditions.checkState(validationAndRetentionConfig != null, "Validation Config is null");
    _replication = validationAndRetentionConfig.getReplicationNumber();
+    // Number of replicas per partition of low-level consumers check is for the real time tables only
+    if (validationAndRetentionConfig.getReplicasPerPartition() != null) {


We need to check if the table is offline or real-time and then decide where to pull the replication data.

Is it possible to wrap all the logic for whether to use replication or replicasPerPartition within the TableConfig itself? That way, tableConfig.getReplicationNumber should just return the correct value depending on whether it is offline or realtime table.

we saw real time tables using balanced strategy in Production. @jugomezv can you please confirm

@navina We can wire that in table config but it would also mean cleaning up at all the other places we use this. I would so keep that out of scope for this fix

Please tag this PR with the (open) issue #8804.

Since we have a problem in production, we should fix in some way (add TODOs as needed), but fix the overall replication/replicasPerPartition as a part of that issue.

codecov-commenter · 2022-11-16T21:18:24Z

Codecov Report

Merging #9816 (f95ac08) into master (47c8f18) will increase coverage by 9.68%.
The diff coverage is 86.27%.

@@             Coverage Diff              @@
##             master    #9816      +/-   ##
============================================
+ Coverage     60.58%   70.26%   +9.68%     
+ Complexity     5281     4998     -283     
============================================
  Files          1949     1964      +15     
  Lines        104632   105033     +401     
  Branches      15847    15896      +49     
============================================
+ Hits          63389    73803   +10414     
+ Misses        36540    26096   -10444     
- Partials       4703     5134     +431

Flag	Coverage Δ
integration1	`25.27% <2.94%> (?)`
integration2	`24.68% <0.98%> (+0.24%)`	⬆️
unittests1	`67.86% <85.41%> (+0.05%)`	⬆️
unittests2	`15.77% <86.27%> (?)`

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

Impacted Files	Coverage Δ
...apache/pinot/common/function/FunctionRegistry.java	`86.84% <ø> (ø)`
...not/query/planner/logical/RelToStageConverter.java	`83.33% <ø> (+3.70%)`	⬆️
...che/pinot/query/planner/logical/RexExpression.java	`81.33% <ø> (+5.33%)`	⬆️
...ache/pinot/query/planner/logical/StagePlanner.java	`100.00% <ø> (ø)`
...e/pinot/query/runtime/operator/FilterOperator.java	`76.00% <ø> (+16.00%)`	⬆️
...ery/runtime/operator/operands/FunctionOperand.java	`100.00% <ø> (ø)`
...query/runtime/operator/MailboxReceiveOperator.java	`79.36% <50.00%> (-0.97%)`	⬇️
...query/runtime/operator/operands/FilterOperand.java	`83.90% <50.00%> (+16.05%)`	⬆️
...va/org/apache/pinot/query/runtime/QueryRunner.java	`86.58% <83.33%> (+8.90%)`	⬆️
...ot/query/runtime/executor/RoundRobinScheduler.java	`87.50% <87.50%> (ø)`
... and 397 more

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

snleee · 2022-11-16T21:39:07Z

Can we also add the testing case to cover this? We should add the test where we set replication / numReplicasPerPartition differently.

snleee · 2022-11-16T21:50:04Z

.../controller/helix/core/assignment/segment/strategy/BalancedNumSegmentAssignmentStrategy.java

@@ -51,6 +52,10 @@ public void init(HelixManager helixManager, TableConfig tableConfig) {
    SegmentsValidationAndRetentionConfig validationAndRetentionConfig = tableConfig.getValidationConfig();
    Preconditions.checkState(validationAndRetentionConfig != null, "Validation Config is null");
    _replication = validationAndRetentionConfig.getReplicationNumber();
+    // Number of replicas per partition of low-level consumers check is for the real time tables only
+    if (tableConfig.getTableType() == TableType.REALTIME) {


Let's add the TODO comment that we can clean this up once the table config has the new API that picks up the correct replication depending on the table type.

added a todo and a unit test.

sajjad-moradi

Looks good. Could you add a unit test?

sajjad-moradi · 2022-11-16T21:55:22Z

.../controller/helix/core/assignment/segment/strategy/BalancedNumSegmentAssignmentStrategy.java

    _replication = validationAndRetentionConfig.getReplicationNumber();
+    // Number of replicas per partition of low-level consumers check is for the real time tables only
+    if (tableConfig.getTableType() == TableType.REALTIME) {
+      _replication = validationAndRetentionConfig.getReplicasPerPartitionNumber();
+    }


nit:

_replication = tableConfig.getTableType() == TableType.REALTIME ? validationAndRetentionConfig.getReplicasPerPartitionNumber() : validationAndRetentionConfig.getReplicationNumber();

snleee

LGTM

snleee reviewed Nov 16, 2022

View reviewed changes

GSharayu force-pushed the new_master branch from 212da59 to 0d53147 Compare November 16, 2022 21:04

snleee reviewed Nov 16, 2022

View reviewed changes

sajjad-moradi approved these changes Nov 16, 2022

View reviewed changes

GSharayu force-pushed the new_master branch 3 times, most recently from 9edbf3f to ee3aab0 Compare November 16, 2022 23:33

Fix the replication in segment assignment

f95ac08

GSharayu force-pushed the new_master branch from ee3aab0 to f95ac08 Compare November 17, 2022 00:18

GSharayu closed this Nov 17, 2022

GSharayu reopened this Nov 17, 2022

GSharayu closed this Nov 17, 2022

GSharayu reopened this Nov 17, 2022

snleee approved these changes Nov 17, 2022

View reviewed changes

snleee merged commit 6d49424 into apache:master Nov 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the replication in segment assignment strategy #9816

Fix the replication in segment assignment strategy #9816

GSharayu commented Nov 16, 2022 •

edited

Loading

GSharayu commented Nov 16, 2022 •

edited

Loading

snleee left a comment •

edited

Loading

snleee Nov 16, 2022

navina Nov 16, 2022

GSharayu Nov 16, 2022 •

edited

Loading

GSharayu Nov 16, 2022

mcvsubbu Nov 16, 2022

GSharayu Nov 16, 2022

codecov-commenter commented Nov 16, 2022 •

edited

Loading

snleee commented Nov 16, 2022

snleee Nov 16, 2022

GSharayu Nov 16, 2022

sajjad-moradi left a comment

sajjad-moradi Nov 16, 2022

snleee Nov 16, 2022

GSharayu Nov 16, 2022

snleee left a comment

Fix the replication in segment assignment strategy #9816

Fix the replication in segment assignment strategy #9816

Conversation

GSharayu commented Nov 16, 2022 • edited Loading

GSharayu commented Nov 16, 2022 • edited Loading

snleee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSharayu Nov 16, 2022 • edited Loading

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

codecov-commenter commented Nov 16, 2022 • edited Loading

Codecov Report

snleee commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sajjad-moradi 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

snleee left a comment

Choose a reason for hiding this comment

GSharayu commented Nov 16, 2022 •

edited

Loading

GSharayu commented Nov 16, 2022 •

edited

Loading

snleee left a comment •

edited

Loading

GSharayu Nov 16, 2022 •

edited

Loading

codecov-commenter commented Nov 16, 2022 •

edited

Loading