You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/12/07 00:15:55 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9662: Table rebalance status
Jackie-Jiang commented on code in PR #9662:
URL: https://github.com/apache/pinot/pull/9662#discussion_r1041609625
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1072,4 +1075,34 @@ private SuccessResponse updateZKTimeIntervalInternal(String tableNameWithType) {
}
return new SuccessResponse("Successfully updated time interval zk metadata for table: " + tableNameWithType);
}
+
+ @GET
+ @Path("/segments/{tableName}/externalViewMismatch")
+ @ApiOperation(value = "Table segment mismatch after rebalance", notes = "Get segments without the same state in EV "
+ + "and IS ")
+ public String getExternalViewSegementMismatch(
+ @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
+ @ApiParam(value = "OFFLINE|REALTIME", required = true) @QueryParam("type") String tableTypeStr) {
+ 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));
+ }
+
+ Map<String, MapDifference.ValueDifference<Map<String, String>>> view =
+ _pinotHelixResourceManager.getExternalViewSegementMismatch(tableNameWithType);
+ ObjectNode data = JsonUtils.newObjectNode();
+ data.put("segments", view.toString());
Review Comment:
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"
}
}
}
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -421,13 +421,13 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
if (InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig, instancePartitionsType)) {
if (reassignInstances) {
String rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
- boolean hasPreConfiguredInstancePartitions = TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig,
- instancePartitionsType);
+ boolean hasPreConfiguredInstancePartitions =
Review Comment:
(minor) Revert the changes in this file since it is not related
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -732,6 +732,7 @@ private String constructTableNameWithType(String tableName, String tableTypeStr)
try {
tableType = TableType.valueOf(tableTypeStr.toUpperCase());
} catch (Exception e) {
+
Review Comment:
(minor) Revert this change
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotSegmentRestletResourceTest.java:
##########
@@ -195,6 +195,32 @@ private void checkCrcRequest(Map<String, SegmentMetadata> metadataTable, int exp
assertEquals(crcMap.size(), expectedSize);
}
+ @Test
+ public void checkTableSegmentMismatch()
+ throws Exception {
+ String tableName = "testTable1";
+ String createTableUrl = TEST_INSTANCE.getControllerRequestURLBuilder().forTableCreate();
+ TableConfigBuilder offlineBuilder = new TableConfigBuilder(TableType.OFFLINE);
+
+ TableConfig testTableConfig = offlineBuilder.setTableName(tableName).build();
+
+ // Create the table
+ ControllerTest.sendPostRequest(createTableUrl, testTableConfig.toJsonString());
+
+ // Rebalance table
+ ControllerTest.sendPostRequest(
Review Comment:
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
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1072,4 +1075,34 @@ private SuccessResponse updateZKTimeIntervalInternal(String tableNameWithType) {
}
return new SuccessResponse("Successfully updated time interval zk metadata for table: " + tableNameWithType);
}
+
+ @GET
+ @Path("/segments/{tableName}/externalViewMismatch")
+ @ApiOperation(value = "Table segment mismatch after rebalance", notes = "Get segments without the same state in EV "
+ + "and IS ")
Review Comment:
(minor) Let's not mention rebalance here
```suggestion
@ApiOperation(value = "Get segments with mismatched state in EV and IS",
notes = "Get segments without the same state in EV and IS ")
```
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -416,7 +414,54 @@ public void testRebalanceWithTiers()
_helixResourceManager.deleteOfflineTable(TIERED_TABLE_NAME);
}
+ @Test
Review Comment:
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.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1072,4 +1075,34 @@ private SuccessResponse updateZKTimeIntervalInternal(String tableNameWithType) {
}
return new SuccessResponse("Successfully updated time interval zk metadata for table: " + tableNameWithType);
}
+
+ @GET
+ @Path("/segments/{tableName}/externalViewMismatch")
+ @ApiOperation(value = "Table segment mismatch after rebalance", notes = "Get segments without the same state in EV "
+ + "and IS ")
+ public String getExternalViewSegementMismatch(
+ @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
+ @ApiParam(value = "OFFLINE|REALTIME", required = true) @QueryParam("type") String tableTypeStr) {
+ 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));
+ }
+
Review Comment:
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org