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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
