Jackie-Jiang commented on code in PR #13372:
URL: https://github.com/apache/pinot/pull/13372#discussion_r1638963244
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -697,6 +698,31 @@ public RebalanceResult rebalance(
}
}
+ /** Waits for jobId to be persisted using a retry policy. Tables with 100k+
segments
+ * take up to a few seconds for the jobId to persist. This ensures the
jobId is present
+ * before returning the jobId to the caller, so they can correctly poll the
jobId. **/
Review Comment:
(minor, convention)
```suggestion
/**
* Waits for jobId to be persisted using a retry policy.
* Tables with 100k+ segments take up to a few seconds for the jobId to
persist. This ensures the jobId is present
* before returning the jobId to the caller, so they can correctly poll
the jobId.
**/
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -697,6 +698,31 @@ public RebalanceResult rebalance(
}
}
+ /** Waits for jobId to be persisted using a retry policy. Tables with 100k+
segments
+ * take up to a few seconds for the jobId to persist. This ensures the
jobId is present
+ * before returning the jobId to the caller, so they can correctly poll the
jobId. **/
+ public void waitForJobIdToPersist(String jobId, String tableNameWithType) {
+ try {
+ // This retry policy waits at most for 3.5 to 7s in total. This is
chosen to cover
+ // typical delays for tables with many segments and avoid excessive
HTTP request timeouts.
+ RetryPolicies.exponentialBackoffRetryPolicy(4, 500L, 2.0)
+ .attempt(() -> {
+ Map<String, String> controllerJobZKMetadata =
getControllerJobMetadata(jobId);
+ if (controllerJobZKMetadata == null) {
+ LOGGER.warn("jobId not persisted yet while rebalancing table:
{}", tableNameWithType);
+ return false;
+ }
Review Comment:
Suggest removing this warning log as it could be very common for the first
attempt to fail. The warning in the exception catch should be good enough
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -697,6 +698,31 @@ public RebalanceResult rebalance(
}
}
+ /** Waits for jobId to be persisted using a retry policy. Tables with 100k+
segments
+ * take up to a few seconds for the jobId to persist. This ensures the
jobId is present
+ * before returning the jobId to the caller, so they can correctly poll the
jobId. **/
+ public void waitForJobIdToPersist(String jobId, String tableNameWithType) {
+ try {
+ // This retry policy waits at most for 3.5 to 7s in total. This is
chosen to cover
+ // typical delays for tables with many segments and avoid excessive
HTTP request timeouts.
+ RetryPolicies.exponentialBackoffRetryPolicy(4, 500L, 2.0)
Review Comment:
We can consider adding one more retry to make it more robust
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -697,6 +698,31 @@ public RebalanceResult rebalance(
}
}
+ /** Waits for jobId to be persisted using a retry policy. Tables with 100k+
segments
+ * take up to a few seconds for the jobId to persist. This ensures the
jobId is present
+ * before returning the jobId to the caller, so they can correctly poll the
jobId. **/
+ public void waitForJobIdToPersist(String jobId, String tableNameWithType) {
+ try {
Review Comment:
(code format) Can you set up [Pinot
Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#set-up-ide)
and reformat the change
--
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]
