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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
