Skip to content

Generate a single routing table if replication is 1#4926

Closed
siddharthteotia wants to merge 1 commit intoapache:masterfrom
siddharthteotia:routing
Closed

Generate a single routing table if replication is 1#4926
siddharthteotia wants to merge 1 commit intoapache:masterfrom
siddharthteotia:routing

Conversation

@siddharthteotia
Copy link
Contributor

If the replication is 1, we don't have to generate different versions of routing tables as all of them will be same

@codecov-io
Copy link

Codecov Report

Merging #4926 into master will increase coverage by 0.11%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4926      +/-   ##
============================================
+ Coverage     56.54%   56.66%   +0.11%     
  Complexity       53       53              
============================================
  Files          1177     1177              
  Lines         62894    62903       +9     
  Branches       9231     9233       +2     
============================================
+ Hits          35566    35644      +78     
+ Misses        24688    24619      -69     
  Partials       2640     2640
Impacted Files Coverage Δ Complexity Δ
...ing/builder/BalancedRandomRoutingTableBuilder.java 92.85% <50%> (-7.15%) 0 <0> (ø)
...ing/builder/GeneratorBasedRoutingTableBuilder.java 98.78% <90%> (-1.22%) 0 <0> (ø)
...elix/core/relocation/RealtimeSegmentRelocator.java 25% <0%> (-10%) 0% <0%> (ø)
...impl/dictionary/FloatOffHeapMutableDictionary.java 52.68% <0%> (-7.53%) 0% <0%> (ø)
...impl/dictionary/DoubleOnHeapMutableDictionary.java 46.34% <0%> (-4.88%) 0% <0%> (ø)
.../broker/routing/HelixExternalViewBasedRouting.java 86.19% <0%> (-3.07%) 0% <0%> (ø)
...not/broker/broker/helix/ClusterChangeMediator.java 74.72% <0%> (-2.2%) 0% <0%> (ø)
...ache/pinot/common/metadata/ZKMetadataProvider.java 66.29% <0%> (-1.69%) 0% <0%> (ø)
...ata/manager/realtime/RealtimeTableDataManager.java 44.61% <0%> (-1.54%) 0% <0%> (ø)
...lix/core/realtime/PinotRealtimeSegmentManager.java 81.02% <0%> (-1.03%) 0% <0%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acaa78d...3fdbb38. Read the comment docs.

super.init(configuration, tableConfig, propertyStore, brokerMetrics);
_numRoutingTables = configuration.getInt(NUM_ROUTING_TABLES_KEY, DEFAULT_NUM_ROUTING_TABLES);
int numReplicas = tableConfig.getValidationConfig().getReplicationNumber();
_numRoutingTables = numReplicas == 1 ? 1 : configuration.getInt(NUM_ROUTING_TABLES_KEY, DEFAULT_NUM_ROUTING_TABLES);
Copy link
Member

Choose a reason for hiding this comment

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

where is this configuration coming from? My main concern about this change is if the replicationFactor changes and we invoke rebalance, the number of routingTable remains the same

Copy link
Contributor Author

@siddharthteotia siddharthteotia Dec 13, 2019

Choose a reason for hiding this comment

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

The replication is coming from tableConfig -- segmentsConfig part of tableConfig.

"segmentsConfig": {
      "completionConfig": null, 
      "hllConfig": null, 
      "replicaGroupStrategyConfig": {
        "numInstancesPerPartition": 2, 
        "partitionColumn": null
      }, 
      "replicasPerPartition": null, 
      "replication": "3",
      "retentionTimeUnit": "DAYS", 
      "retentionTimeValue": "7320", 
    }

If the replication changes, the change should be reflected in table config right? So on the next change to external view (after changing the table config and ideal state with new servers, rebalance etc), the routing table builder should see the updated config and thus the new replication factor. Is my understanding correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just FYI.... we had seen this in couple of non prod deployments where there were 1000s of segments across 20+ servers but replication 1 and the routing table looked weird with duplicate server -> segments mapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are not guaranteed to see the updated table config after the replication factor changes then yes we should not be doing this change. AFAIK, there is no way to change the replication factor without going via table config

@kishoreg
Copy link
Member

Broker nodes are not listening to table config changes. But external view has the replication factor if I am not mistaken. So my suggestion is to use that value to compute the number of routing tables.

@Jackie-Jiang
Copy link
Contributor

No longer apply

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants