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/07/11 21:29:04 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8828: Add controller API for reload segment task status
Jackie-Jiang commented on code in PR #8828:
URL: https://github.com/apache/pinot/pull/8828#discussion_r918342988
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,9 +429,17 @@ public SuccessResponse reloadSegment(
TableType tableType = SegmentName.isRealtimeSegmentName(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
String tableNameWithType =
ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableType, LOGGER).get(0);
- int numMessagesSent = _pinotHelixResourceManager.reloadSegment(tableNameWithType, segmentName, forceDownload);
- if (numMessagesSent > 0) {
- return new SuccessResponse("Sent " + numMessagesSent + " reload messages");
+ Pair<Integer, String> msgInfo =
+ _pinotHelixResourceManager.reloadSegment(tableNameWithType, segmentName, forceDownload);
+ if (msgInfo.getLeft() > 0) {
+ try {
+ _pinotHelixResourceManager.addNewReloadSegmentJob(tableNameWithType, segmentName, msgInfo.getRight(),
+ msgInfo.getLeft());
+ } catch (Exception e) {
+ LOGGER.error("Failed to add reload segment job meta into zookeeper for table {}, segment {}",
+ tableNameWithType, segmentName, e);
+ }
+ return new SuccessResponse("Sent " + msgInfo + " reload messages");
Review Comment:
Then let's change the response message to include both job id and the messages sent, something like `"Submitted reload job: %s, sent %d reload messages"`
##########
pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java:
##########
@@ -109,6 +110,10 @@ public static String constructPropertyStorePathForInstancePartitions(String inst
return StringUtil.join("/", PROPERTYSTORE_INSTANCE_PARTITIONS_PREFIX, instancePartitionsName);
}
+ public static String constructPropertyStorePathForControllerJob(String resourceName) {
+ return StringUtil.join("/", PROPERTYSTORE_CONTROLLER_JOBS_PREFIX, resourceName);
Review Comment:
This will create one ZNode per table, which is too much IMO. We are just storing the recent controller jobs in the ZNode, and there shouldn't be a lot of them running in parallel, so we can have one single ZNode for all the tables. Keeping one single node will help track all the running jobs, and also clean up the old jobs.
We can also consider having one node per job type to isolate the metadata for different jobs. All the entries for the same job type should have the same format, which is easier to manage
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -563,14 +662,24 @@ public SuccessResponse reloadAllSegments(
if (forceDownload && (tableTypeFromTableName == null && tableTypeFromRequest == null)) {
tableTypeFromRequest = TableType.OFFLINE;
}
- List<String> tableNamesWithType = ResourceUtils
- .getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableTypeFromRequest, LOGGER);
- Map<String, Integer> numMessagesSentPerTable = new LinkedHashMap<>();
+ List<String> tableNamesWithType =
+ ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableTypeFromRequest,
+ LOGGER);
+ Map<String, Pair<Integer, String>> perTableMsgData = new LinkedHashMap<>();
for (String tableNameWithType : tableNamesWithType) {
- int numMsgSent = _pinotHelixResourceManager.reloadAllSegments(tableNameWithType, forceDownload);
- numMessagesSentPerTable.put(tableNameWithType, numMsgSent);
+ Pair<Integer, String> msgInfo = _pinotHelixResourceManager.reloadAllSegments(tableNameWithType, forceDownload);
+ perTableMsgData.put(tableNameWithType, msgInfo);
+ // Store in ZK
+ try {
+ if (!_pinotHelixResourceManager.addNewReloadAllSegmentsJob(tableNameWithType, msgInfo.getRight(),
+ msgInfo.getLeft())) {
+ LOGGER.error("Failed to add reload all segments job meta into zookeeper for table {}", tableNameWithType);
+ }
+ } catch (Exception e) {
+ LOGGER.error("Failed to add reload all segments job meta into zookeeper for table {}", tableNameWithType, e);
+ }
}
- return new SuccessResponse("Sent " + numMessagesSentPerTable + " reload messages");
+ return new SuccessResponse("Sent " + perTableMsgData + " reload messages");
Review Comment:
Let's make the response more clear to the user
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -425,9 +429,17 @@ public SuccessResponse reloadSegment(
TableType tableType = SegmentName.isRealtimeSegmentName(segmentName) ? TableType.REALTIME : TableType.OFFLINE;
String tableNameWithType =
ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableType, LOGGER).get(0);
- int numMessagesSent = _pinotHelixResourceManager.reloadSegment(tableNameWithType, segmentName, forceDownload);
- if (numMessagesSent > 0) {
- return new SuccessResponse("Sent " + numMessagesSent + " reload messages");
+ Pair<Integer, String> msgInfo =
+ _pinotHelixResourceManager.reloadSegment(tableNameWithType, segmentName, forceDownload);
+ if (msgInfo.getLeft() > 0) {
+ try {
+ _pinotHelixResourceManager.addNewReloadSegmentJob(tableNameWithType, segmentName, msgInfo.getRight(),
+ msgInfo.getLeft());
+ } catch (Exception e) {
+ LOGGER.error("Failed to add reload segment job meta into zookeeper for table {}, segment {}",
Review Comment:
^^
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -563,14 +662,24 @@ public SuccessResponse reloadAllSegments(
if (forceDownload && (tableTypeFromTableName == null && tableTypeFromRequest == null)) {
tableTypeFromRequest = TableType.OFFLINE;
}
- List<String> tableNamesWithType = ResourceUtils
- .getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableTypeFromRequest, LOGGER);
- Map<String, Integer> numMessagesSentPerTable = new LinkedHashMap<>();
+ List<String> tableNamesWithType =
+ ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableTypeFromRequest,
+ LOGGER);
+ Map<String, Pair<Integer, String>> perTableMsgData = new LinkedHashMap<>();
for (String tableNameWithType : tableNamesWithType) {
- int numMsgSent = _pinotHelixResourceManager.reloadAllSegments(tableNameWithType, forceDownload);
- numMessagesSentPerTable.put(tableNameWithType, numMsgSent);
+ Pair<Integer, String> msgInfo = _pinotHelixResourceManager.reloadAllSegments(tableNameWithType, forceDownload);
+ perTableMsgData.put(tableNameWithType, msgInfo);
+ // Store in ZK
+ try {
+ if (!_pinotHelixResourceManager.addNewReloadAllSegmentsJob(tableNameWithType, msgInfo.getRight(),
+ msgInfo.getLeft())) {
+ LOGGER.error("Failed to add reload all segments job meta into zookeeper for table {}", tableNameWithType);
+ }
+ } catch (Exception e) {
+ LOGGER.error("Failed to add reload all segments job meta into zookeeper for table {}", tableNameWithType, e);
Review Comment:
Reflect the errors back to the user
--
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