Table rebalance status by antwigambrah · Pull Request #9662 · apache/pinot

antwigambrah · 2022-10-26T04:09:38Z

Description:
This PR adds an api for checking table rebalance status

This PR will solve the following issue --#9558

codecov-commenter · 2022-10-26T04:58:06Z

Codecov Report

Merging #9662 (f8eb0e1) into master (7eba70c) will decrease coverage by 45.30%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master    #9662       +/-   ##
=============================================
- Coverage     70.42%   25.12%   -45.31%     
+ Complexity     5662       44     -5618     
=============================================
  Files          1991     1979       -12     
  Lines        107251   106924      -327     
  Branches      16302    16269       -33     
=============================================
- Hits          75533    26861    -48672     
- Misses        26444    77351    +50907     
+ Partials       5274     2712     -2562

Flag	Coverage Δ
integration1	`25.12% <0.00%> (-0.05%)`	⬇️
integration2	`?`
unittests1	`?`
unittests2	`?`

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

Impacted Files	Coverage Δ
...ler/api/resources/PinotSegmentRestletResource.java	`32.13% <0.00%> (-7.59%)`	⬇️
...ntroller/helix/core/PinotHelixResourceManager.java	`39.56% <0.00%> (-31.60%)`	⬇️
...ntroller/helix/core/rebalance/TableRebalancer.java	`53.82% <0.00%> (-22.14%)`	⬇️
...spi/utils/builder/ControllerRequestURLBuilder.java	`0.00% <0.00%> (ø)`
...ain/java/org/apache/pinot/spi/utils/LoopUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...in/java/org/apache/pinot/spi/utils/BytesUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pinot/spi/config/user/RoleType.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1449 more

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

siddharthteotia · 2022-10-27T07:03:19Z

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

+    if (idealState == null) {
+      throw new IllegalStateException("ideal state is empty");
+    }
+    if (externalView != null && isExternalViewConverged(tableNameWithType, externalView.getRecord().getMapFields(),


Do we want to consider the situation where external view never converges with what ongoing rebalancer expects or external view ends up having segments in ERROR state ?

Looks like we will continue returning IN_PROGRESS in such scenarios and that could be misleading ? Probably also depends on what is the call to action for the user/admin who uses this API...

HI @siddharthteotia, sorry I updated the above code before reading this comment.Currently, the external view returns false if it is not converged yet and logs an error if the external view ends up having segments in error .I believe it is best to return the status FAILED instead of logging. Also, would it be ok to return NO_OP if the external view is still null when the api is called .cc @Jackie-Jiang

siddharthteotia · 2022-10-29T00:22:38Z

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

+
+    if (isExternalViewConverged(tableNameWithType, externalView.getRecord().getMapFields(),
+        idealState.getRecord().getMapFields(), true)) {
+      return RebalanceResult.Status.DONE;


I am not sure if it makes sense / helpful enough to sort of implement a stateless API to get status of rebalance task -- which was triggered separately through some other API and is being done asynchronously on potentially a different controller that getStatus() API call comes on

Relying on the fact that ExternalView is not the same as IdealState to indicate Rebalance is IN_PROGRESS could be misleading.

Segment assignment changes ideal state as soon as segment lands on controller and up until server successfully downloads the segment etc, ExternalView does not get updated. But this API will return rebalance is in IN_PROGRESS even though rebalance may have already finished or could have errored out etc.

So not sure if this is going to be super helpful / accurate.

cc @antwigambrah @Jackie-Jiang thoughts ?

HI, @siddharthteotia Yes it will be quite misleading. Checking for IN_PROGRESS seems tricky here.

The possible way I can think for now through testing I have done on TableRebalancerClusterStatelessTest is to check on the external view from being null to being allocated segments assignments.

For DONE status, I will run the rebalance in dry mode and retrieve the new assignments from the result and use is checking if the extervalViewConverged . Kindly let me know you thoughts on it . cc @Jackie-Jiang

Jackie-Jiang

The API added in this PR basically checks if the table's EV matches IS. This itself cannot represent the status of rebalance because rebalance can be done in multiple steps, and in the middle EV can match IS. But this is also a quite useful endpoint for status tracking purpose.
I'd suggest changing the API to return whether the table has EV converged, and if not, list down the segments that do not have the same state in EV and IS.

antwigambrah · 2022-11-11T12:00:10Z

@Jackie-Jiang Please review with the new update

Jackie-Jiang · 2022-11-21T19:03:51Z

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

+      @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
+      @ApiParam(value = "OFFLINE|REALTIME", required = true) @QueryParam("type") String tableTypeStr) {
+    try {
+        String tableNameWithType = constructTableNameWithType(tableName, tableTypeStr);


(code format) Please reformat the changes with Pinot Style

Jackie-Jiang · 2022-11-21T19:18:31Z

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

  }

+  @GET
+  @Path("/tables/{tableName}/rebalance/status")


Let's make the API more specific, and it might be better to put it under the segment rest API: /segments/{tableName}/externalViewMismatch and returns a map from the mismatch segment to its ideal state and external view

Jackie-Jiang · 2022-11-21T19:19:11Z

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


+  public Map<String, MapDifference.ValueDifference<Map<String, String>>> rebalanceTableStatus(String tableNameWithType)
+      throws TableNotFoundException {
+    TableConfig tableConfig = getTableConfig(tableNameWithType);


No need to read the table config. We should check both IS and EV exist

Jackie-Jiang · 2022-11-21T19:20:22Z

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

    }
  }

+  public Map<String, MapDifference.ValueDifference<Map<String, String>>> rebalanceStatus(String tableNameWithType,


Don't put this into the TableRebalancer. We can directly have it in PinotHelixResourceManager

Jackie-Jiang · 2022-11-21T19:21:34Z

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

    try {
      tableType = TableType.valueOf(tableTypeStr.toUpperCase());
    } catch (Exception e) {
+


(nit) Please revert some unnecessary empty line changes in this PR

…alance_status

antwigambrah · 2022-12-05T03:28:59Z

Hi @Jackie-Jiang , Please review with the new update

Jackie-Jiang

The overall direction looks good

Jackie-Jiang · 2022-12-06T23:51:48Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

+  @ApiOperation(value = "Table segment mismatch after rebalance", notes = "Get segments without the same state in EV "
+      + "and IS ")


(minor) Let's not mention rebalance here

Suggested change

@ApiOperation(value = "Table segment mismatch after rebalance", notes = "Get segments without the same state in EV "

+ "and IS ")

@ApiOperation(value = "Get segments with mismatched state in EV and IS",

notes = "Get segments without the same state in EV and IS ")

Jackie-Jiang · 2022-12-06T23:55:44Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

+    try {
+      TableType tableType = Constants.validateTableType(tableTypeStr);
+
+      if (tableType == null) {
+        throw new ControllerApplicationException(LOGGER, "Table type must not be null", Status.BAD_REQUEST);
+      }
+      String tableNameWithType = TableNameBuilder.forType(TableType.valueOf(tableTypeStr)).tableNameWithType(tableName);
+
+      if (!_pinotHelixResourceManager.hasTable(tableNameWithType)) {
+        throw new TableNotFoundException(String.format("Table=%s not found", tableName));
+      }
+


We already have util to perform the same checks. You may refer to listSegmentLineage()

TableType tableType = Constants.validateTableType(tableTypeStr); if (tableType == null) { throw new ControllerApplicationException(LOGGER, "Table type should either be offline or realtime", Response.Status.BAD_REQUEST); } String tableNameWithType = ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableType, LOGGER).get(0);

Jackie-Jiang · 2022-12-06T23:58:18Z

...va/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java


    _helixResourceManager.deleteOfflineTable(TIERED_TABLE_NAME);
  }
+  @Test


Suggest not adding the rebalance status check test in this PR. This PR is focusing on the EV mismatch, and we can use it to build the rebalance status check in a separate PR and add the test there.

Jackie-Jiang · 2022-12-07T00:05:04Z

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

    try {
      tableType = TableType.valueOf(tableTypeStr.toUpperCase());
    } catch (Exception e) {
+


(minor) Revert this change

Jackie-Jiang · 2022-12-07T00:05:41Z

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

        String rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
-        boolean hasPreConfiguredInstancePartitions = TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig,
-            instancePartitionsType);
+        boolean hasPreConfiguredInstancePartitions =


(minor) Revert the changes in this file since it is not related

Jackie-Jiang · 2022-12-07T00:11:05Z

...ontroller/src/test/java/org/apache/pinot/controller/api/PinotSegmentRestletResourceTest.java

+    ControllerTest.sendPostRequest(createTableUrl, testTableConfig.toJsonString());
+
+    // Rebalance table
+    ControllerTest.sendPostRequest(


There is no segment in this table, so both rebalance and segment mismatch will be no-op. Let's try to generate a non-empty IS and EV with some diff, and verify the behavior

Jackie-Jiang · 2022-12-07T00:14:54Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

+      Map<String, MapDifference.ValueDifference<Map<String, String>>> view =
+          _pinotHelixResourceManager.getExternalViewSegementMismatch(tableNameWithType);
+      ObjectNode data = JsonUtils.newObjectNode();
+      data.put("segments", view.toString());


I feel we might want the response to be the following format (segmentName -> EV/IS -> different instanceState):

{ "segment0": { "IdealState": { "server0": "ONLINE", "server1": "ONLINE" }, "externalView": { "server1": "ERROR", "server2": "ONLINE" } }, "segment1": { "IdealState": { "server1": "ONLINE" }, "externalView": { "server1": "ERROR" } } }

antwigambrah · 2022-12-16T06:33:21Z

Hi @Jackie-Jiang , an update has been provided for this pr. Kindly review

Jackie-Jiang · 2023-07-11T01:55:45Z

Decided to move forward with #10359

gantwi1 added 2 commits October 26, 2022 02:44

add table rebalance status checker

c824d2d

fix formatting

dee62c0

siddharthteotia reviewed Oct 27, 2022

View reviewed changes

gantwi1 added 3 commits October 27, 2022 08:16

update tests

fa7b75b

remove print

90d7551

fix style checks

08ad4bb

siddharthteotia reviewed Oct 29, 2022

View reviewed changes

Jackie-Jiang reviewed Nov 1, 2022

View reviewed changes

gantwi1 added 4 commits November 9, 2022 13:59

add ev and is state diff

d7df3a2

Merge branch 'apache:master' into table_rebalance_status

e6e656d

Merge branch 'apache:master' into table_rebalance_status

0289065

fix updated var

01d9da7

Jackie-Jiang reviewed Nov 21, 2022

View reviewed changes

gantwi1 added 4 commits November 29, 2022 22:33

Merge branch 'apache:master' into table_rebalance_status

7a73f3d

update endpoint

945dbeb

fix failing tests

b026752

Merge branch 'master' of github.com:antwigambrah/pinot into table_reb…

80579a7

…alance_status

Jackie-Jiang reviewed Dec 7, 2022

View reviewed changes

gantwi1 added 2 commits December 16, 2022 00:25

update mismatch response

b08126d

Merge branch 'master' into table_rebalance_status

f8eb0e1

Jackie-Jiang closed this Jul 11, 2023

		@ApiOperation(value = "Table segment mismatch after rebalance", notes = "Get segments without the same state in EV "
		+ "and IS ")

Conversation

antwigambrah commented Oct 26, 2022

Uh oh!

codecov-commenter commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

antwigambrah commented Nov 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antwigambrah commented Dec 5, 2022

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antwigambrah commented Dec 16, 2022

Uh oh!

Jackie-Jiang commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Oct 26, 2022 •

edited

Loading

siddharthteotia Oct 29, 2022 •

edited

Loading