[colocated-join] Adds Support for instancePartitionsMap in Table Config #8989

ankitsultana · 2022-06-29T08:37:05Z

You can find the design doc here: https://docs.google.com/document/d/1HWkRmjUxLBnkKETw21GJ--yQdiFa9hEE4v6Y68nNWx4/edit

I am planning to raise PRs in 4 increments:

Adds support for instancePartitionsMap.
Add support for Colocated Segment Assignment
[Pending Discussion] Add support for "Assignments".
UI Changes

codecov-commenter · 2022-06-29T09:18:02Z

Codecov Report

Merging #8989 (a9ab073) into master (db07f29) will increase coverage by 41.15%.
The diff coverage is 34.84%.

@@              Coverage Diff              @@
##             master    #8989       +/-   ##
=============================================
+ Coverage     28.83%   69.99%   +41.15%     
- Complexity       47     4685     +4638     
=============================================
  Files          1805     1852       +47     
  Lines         94438    98822     +4384     
  Branches      14140    15032      +892     
=============================================
+ Hits          27234    69171    +41937     
+ Misses        64654    24779    -39875     
- Partials       2550     4872     +2322

Flag	Coverage Δ
integration1	`26.32% <6.06%> (-0.20%)`	⬇️
integration2	`24.68% <6.06%> (-0.33%)`	⬇️
unittests1	`67.13% <30.76%> (?)`
unittests2	`15.25% <13.63%> (?)`

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

Impacted Files	Coverage Δ
...mmon/assignment/InstanceAssignmentConfigUtils.java	`14.63% <0.00%> (-0.76%)`	⬇️
...he/pinot/common/assignment/InstancePartitions.java	`40.00% <0.00%> (-1.18%)`	⬇️
...ources/PinotInstanceAssignmentRestletResource.java	`70.58% <0.00%> (+70.58%)`	⬆️
...ntroller/helix/core/rebalance/TableRebalancer.java	`75.95% <0.00%> (+20.46%)`	⬆️
...not/common/assignment/InstancePartitionsUtils.java	`57.14% <11.11%> (-10.36%)`	⬇️
...he/pinot/common/utils/config/TableConfigUtils.java	`82.35% <27.27%> (+22.49%)`	⬆️
...he/pinot/segment/local/utils/TableConfigUtils.java	`67.57% <55.55%> (+67.57%)`	⬆️
...org/apache/pinot/spi/config/table/TableConfig.java	`64.19% <60.00%> (+64.19%)`	⬆️
...ntroller/helix/core/PinotHelixResourceManager.java	`67.84% <90.00%> (+28.92%)`	⬆️
...he/pinot/spi/utils/builder/TableConfigBuilder.java	`83.33% <100.00%> (+83.33%)`	⬆️
... and 1388 more

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

ankitsultana · 2022-06-29T10:10:52Z

...ava/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java

@@ -55,19 +58,36 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
  public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
      List<InstanceConfig> instanceConfigs, @Nullable InstancePartitions existingInstancePartitions) {
    String tableNameWithType = _tableConfig.getTableName();
-    LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+    Preconditions.checkState(!TableConfigUtils.isTableInGroup(_tableConfig));


Are there any concerns with making ZK propertyStore a member of InstanceAssignmentDriver? That would allow to get rid of this Precondition here and return the pre-computed instance-partitions by doing a look-up on the propertyStore here.

Otherwise we will have to do a check whenever we call this method to see if the table is part of a table-group and handle that case every time.

...t-common/src/main/java/org/apache/pinot/common/assignment/InstanceAssignmentConfigUtils.java

pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java

pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableGroupConfigUtils.java

pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/Constants.java

.../src/main/java/org/apache/pinot/controller/api/resources/PinotTableGroupRestletResource.java

...apache/pinot/controller/helix/core/assignment/segment/ColocatedOfflineSegmentAssignment.java

...n/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableGroupConfig.java

.../src/main/java/org/apache/pinot/controller/api/resources/PinotTableGroupRestletResource.java

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java

siddharthteotia · 2022-07-13T00:21:35Z

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java

+    if (StringUtils.isBlank(tableConfig.getTableGroupName())) {
+      return;
+    }
+    Preconditions.checkState(tableConfig.getValidationConfig().getReplicaGroupStrategyConfig() != null, "Must provide"


This check may not be needed.

replicaGroupStrategyConfig is the legacy way (which at some point has to be deleted so let's try not to leak it more in the code) of specifying replica groups and partitioned replica groups config.

Since adding tables to TableGroup is wired via TableGroupConfig and the instanceAssignmentConfig within TableGroupConfig, it is sufficient and you don't need to enforce that each table within the TableGroup should also have their own replicaGroupStrategyConfig.

Also, the current check does not really ensure that table is indeed partitioned. So you need to check for segmentPartitionConfig.

The partitioning column info coming out of replicaGroupStrategyConfig is only relevant in the context when a partitioned table is partitioned at the replica group level as well. In any case, InstanceAssignmentConfig / InstanceReplicalGroupPartitionConfig handle both scenarios.

Thanks, I was also confused about replicaGroupStrategyConfig. I have removed it from the validation logic now and instead check for column partition map.

siddharthteotia · 2022-07-13T00:57:29Z

Just left a few comments. Still reviewing rest of the changes.

siddharthteotia · 2022-07-13T01:13:54Z

...ava/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java

+    return assignInstances(instancePartitionsName, groupName, instanceConfigs, assignmentConfig, null);
+  }
+
+  private static InstancePartitions assignInstances(String instancePartitionsName, String entityName,


Not sure I understand the concept of entityName and why is it needed. Can you please elaborate ?

Yeah so this method is essentially getting called for both a table-group and a table itself, so it could either be a table-group name or a table name. This name is being used for logging and performing hash-based rotations (e.g. HashBasedRotateInstanceConstraintApplier).

siddharthteotia · 2022-07-13T01:51:29Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

@@ -1688,13 +1690,22 @@ private void assignInstances(TableConfig tableConfig, boolean override) {
      }
    }

+    boolean isTableInGroup = TableConfigUtils.isTableInGroup(tableConfig);


What are the scenarios that we support ?

Adding a new table to TableGroup that pre-exists with 1 or more tables.

In such scenario, the replica group based InstancePartitions for the group should pre-exist and when addTable calls comes in PinotHelixResourceManager and internally calls assignInstances, no new instance assignment should happen and the group's existing InstancePartitions should be retrieved and persisted in ZK for the new table. Looks like this is supported ?

Creating a new TableGroup with new tables.

When the group is created, we don't do instance assignment and just do metadata change w.r.t that group exists without any tables and no instances.

When the first new table is created and added to the group, the addTable -> assignInstances call should recognize that this table is part of a group for which instance assignment is yet to happen. So we call the driver to assign instances for the group.

When the 2nd, 3rd ... table is created and added to the group, the addTable -> assignInstances call should recognize that this table is part of a group for which instance assignment is already done.

The other way to do is that when TableGroup is created, it triggers InstanceAssignment by calling the driver. So when 1st, 2nd, 3rd table is added, it finds it is member of an existing group with existing assignment so it is pretty much NO-OP

It seems like code at line 1701 is assuming that InstancePartitions for a TableGroup should exist. But I don't see instance assignment being done during creation of TableGroup. So how do we handle the case of 1st table ?

Furthermore

Creating a new TableGroup out of N existing tables

Option 1 -

Create TableGroup and assign instances so InstancePartitions for the group are computed and persisted

Trigger rebalance on each of the N tables with reassignInstances set to true

The rebalance code will call the instance assignment driver and it should return the existing InstancePartitions for the group.

Segments are then assigned based on the returned InstancePartitions

Option 2 -

Create TableGroup but don't do instance assignment.

Trigger rebalance on each of the N tables with reassignInstances set to true.

The rebalance code will call the instance assignment driver and it should detect that InstancePartitions don't exit for the group, so it assigns instance which are then used for segment assignment.

Adding an existing table to an existing TableGroup

This is like steps 2,3,4 of option 1 mentioned above. Correct ?

It would be good to get clarity (and add brief details to code comments/docs as well) on how such scenarios are handled.

Adding a new table to TableGroup that pre-exists with 1 or more tables.

Yes this is supported. To do this one can edit the table-config for the corresponding table and set the groupName field. You will have to initiate a rebalance on the table to ensure data is moved around appropriately for that table.

The other way to do is that when TableGroup is created, it triggers InstanceAssignment by calling the driver.

Yup this is how I am doing it right now. You can refer to the createTableGroup API in PinotTableGroupRestletResource. When the table-group is created, we perform instance assignment for the same and persist it in ZK for future re-use.

When we need to update the instance assignment for the group (due to addition/removal of instances), we can initiate a "Re-compute Table Group" operation from the UI and will then have to call Rebalance for each of the tables. The "Re-compute" operation will do the instance assignment for the group again and update the same in ZK. When we initiate rebalance for a table in the group, data will move around for that table based on the newly updated instance partitions for the group.

Later, I am also planning to add a "Re-balance Table Group" option in the UI which should move the data for all the tables in the group.

Creating a new TableGroup out of N existing tables

Yes current approach is based on option-1.

Adding an existing table to an existing TableGroup

This is like steps 2,3,4 of option 1 mentioned above. Correct ?

Yes that's correct.

I'll update the code and doc with more details on this.

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java

kishoreg · 2022-07-18T02:35:37Z

Hi @ankitsultana,

Thanks for writing that doc and also making the video. It was very helpful.

I think we can simplify the design a bit and align it with how the other features achieve segment assignment. Please take a look at it and we can discuss on the doc.

Jackie-Jiang

LGTM, only some minor comments

pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java

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

...n/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java

Jackie-Jiang

Some minor non-blocking comments

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java

Add Table Group Code

09302b6

ankitsultana force-pushed the tg-part-1 branch from e0d4ab8 to 09302b6 Compare June 29, 2022 09:56

ankitsultana commented Jun 29, 2022

View reviewed changes

...t-common/src/main/java/org/apache/pinot/common/assignment/InstanceAssignmentConfigUtils.java Outdated Show resolved Hide resolved

ankitsultana marked this pull request as ready for review June 29, 2022 10:12

yupeng9 reviewed Jun 29, 2022

View reviewed changes

Address feedback

831f7a4

ankitsultana force-pushed the tg-part-1 branch from e4dd924 to 831f7a4 Compare June 29, 2022 18:56

ankitsultana changed the title ~~[Draft] Part-1: Add Table Group Ingestion~~ Part-1: Add Table Group Ingestion Jun 29, 2022

mcvsubbu assigned siddharthteotia Jul 2, 2022

ankitsultana commented Jul 12, 2022

View reviewed changes

...n/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Jul 12, 2022

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableGroupConfig.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Jul 13, 2022

View reviewed changes

.../src/main/java/org/apache/pinot/controller/api/resources/PinotTableGroupRestletResource.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Jul 13, 2022

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Jul 13, 2022

View reviewed changes

ankitsultana mentioned this pull request Jul 13, 2022

Add API and UI View for All Tables in a Table-Group #9052

Closed

Jackie-Jiang reviewed Jul 16, 2022

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TableConfig.java Outdated Show resolved Hide resolved

ankitsultana added 8 commits July 22, 2022 14:26

Fix TableGroupConfig validation

d8516aa

Add tests, address some feedback

5347f93

Handle partitioning in replica-groups

3f7c98e

temp

eb471fd

Change based on feedback

dd47fc4

core code

15fe373

Allow assignment to any existing instance partitions

3997ef1

Self-review.. cleanup

847f93f

Remove segment assignment code

c1d7086

ankitsultana changed the title ~~Part-1: Add Table Group Ingestion~~ [colocated-join] Adds Table Group and Reusable Instance Partitions Support Jul 29, 2022

No check needed for allowing instance assignment for group

ae41533

ankitsultana requested review from Jackie-Jiang and yupeng9 July 29, 2022 14:55

ankitsultana added 4 commits August 3, 2022 22:12

Keep only instancePartitionsMap

dc239ac

self-review

1c22825

Revert package-lock.json changes

c615af5

Fix checkstyle

aa54542

ankitsultana changed the title ~~[colocated-join] Adds Table Group and Reusable Instance Partitions Support~~ [colocated-join] Adds Support for instancePartitionsMap in Table Config Aug 4, 2022

Jackie-Jiang approved these changes Aug 5, 2022

View reviewed changes

ankitsultana requested a review from siddharthteotia August 5, 2022 21:54

Address Jackie's Feedback

66b23cf

Jackie-Jiang reviewed Aug 6, 2022

View reviewed changes

Address feedback

a9ab073

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Aug 11, 2022

Jackie-Jiang merged commit 54d2813 into apache:master Aug 11, 2022

Jackie-Jiang mentioned this pull request Sep 17, 2023

pre-configuration based assignment #11578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[colocated-join] Adds Support for instancePartitionsMap in Table Config #8989

[colocated-join] Adds Support for instancePartitionsMap in Table Config #8989

ankitsultana commented Jun 29, 2022 •

edited

Loading

codecov-commenter commented Jun 29, 2022 •

edited

Loading

ankitsultana Jun 29, 2022

siddharthteotia Jul 13, 2022 •

edited

Loading

ankitsultana Jul 25, 2022

siddharthteotia commented Jul 13, 2022

siddharthteotia Jul 13, 2022 •

edited

Loading

ankitsultana Jul 13, 2022

siddharthteotia Jul 13, 2022 •

edited

Loading

siddharthteotia Jul 13, 2022 •

edited

Loading

ankitsultana Jul 13, 2022

ankitsultana Jul 13, 2022

kishoreg commented Jul 18, 2022

Jackie-Jiang left a comment

Jackie-Jiang left a comment

[colocated-join] Adds Support for instancePartitionsMap in Table Config #8989

[colocated-join] Adds Support for instancePartitionsMap in Table Config #8989

Conversation

ankitsultana commented Jun 29, 2022 • edited Loading

codecov-commenter commented Jun 29, 2022 • edited Loading

Codecov Report

ankitsultana Jun 29, 2022

Choose a reason for hiding this comment

siddharthteotia Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

ankitsultana Jul 25, 2022

Choose a reason for hiding this comment

siddharthteotia commented Jul 13, 2022

siddharthteotia Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

ankitsultana Jul 13, 2022

Choose a reason for hiding this comment

siddharthteotia Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

siddharthteotia Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

ankitsultana Jul 13, 2022

Choose a reason for hiding this comment

ankitsultana Jul 13, 2022

Choose a reason for hiding this comment

kishoreg commented Jul 18, 2022

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

ankitsultana commented Jun 29, 2022 •

edited

Loading

codecov-commenter commented Jun 29, 2022 •

edited

Loading

siddharthteotia Jul 13, 2022 •

edited

Loading

siddharthteotia Jul 13, 2022 •

edited

Loading

siddharthteotia Jul 13, 2022 •

edited

Loading

siddharthteotia Jul 13, 2022 •

edited

Loading