xiangfu0 commented on code in PR #20009:
URL: https://github.com/apache/pinot/pull/20009#discussion_r4366205249
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -319,18 +324,45 @@ public synchronized void deleteTaskQueue(String taskType,
boolean forceDelete) {
/// Delete a single task from the task queue. The task queue should be
/// stopped before deleting the task, otherwise it fails with exception.
///
+ /// A delete that fails with [HelixException] is retried a few times with a
growing backoff. The Helix task pipeline
+ /// can still be writing the job context right after the queue is stopped or
the task is scheduled. Removing the
+ /// context ZNode then fails with ZooKeeper `NotEmpty`, which Helix reports
as a [HelixException] without the cause.
+ /// Each step of a delete skips ZNodes that are already gone, so a retry
resumes where the failed attempt stopped.
+ /// Each attempt holds this manager's lock, but the backoff between attempts
does not.
+ ///
/// @param taskName the task to delete from the queue.
/// @param forceDelete as said in helix comment, if set true, all job's
related zk nodes will
/// be clean up from zookeeper even if its workflow
information can not be found.
- public synchronized void deleteTask(String taskName, boolean forceDelete) {
+ public void deleteTask(String taskName, boolean forceDelete) {
Review Comment:
Dropping the method-level `synchronized` lets another thread run between
attempts. If a concurrent `deleteTaskQueue(forceDelete=true)` removes the whole
workflow during a backoff, a non-forced retry would then hit
`IllegalArgumentException` (queue config gone) instead of the original
`HelixException`, so the caller sees a different error. Narrow window and
arguably already racy — just flagging, not blocking.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotHelixTaskResourceManager.java:
##########
@@ -319,18 +324,45 @@ public synchronized void deleteTaskQueue(String taskType,
boolean forceDelete) {
/// Delete a single task from the task queue. The task queue should be
/// stopped before deleting the task, otherwise it fails with exception.
///
+ /// A delete that fails with [HelixException] is retried a few times with a
growing backoff. The Helix task pipeline
+ /// can still be writing the job context right after the queue is stopped or
the task is scheduled. Removing the
+ /// context ZNode then fails with ZooKeeper `NotEmpty`, which Helix reports
as a [HelixException] without the cause.
+ /// Each step of a delete skips ZNodes that are already gone, so a retry
resumes where the failed attempt stopped.
+ /// Each attempt holds this manager's lock, but the backoff between attempts
does not.
+ ///
/// @param taskName the task to delete from the queue.
/// @param forceDelete as said in helix comment, if set true, all job's
related zk nodes will
/// be clean up from zookeeper even if its workflow
information can not be found.
- public synchronized void deleteTask(String taskName, boolean forceDelete) {
+ public void deleteTask(String taskName, boolean forceDelete) {
String taskType = getTaskType(taskName);
String helixJobQueueName = getHelixJobQueueName(taskType);
if (forceDelete) {
LOGGER.warn("Force deleting task: {} from queue: {} of task type: {}",
taskName, helixJobQueueName, taskType);
} else {
LOGGER.info("Deleting task: {} from queue: {} of task type: {}",
taskName, helixJobQueueName, taskType);
}
- _taskDriver.deleteJob(helixJobQueueName, taskName, forceDelete);
+ for (int attempt = 1; ; attempt++) {
+ try {
+ synchronized (this) {
+ _taskDriver.deleteJob(helixJobQueueName, taskName, forceDelete);
+ }
+ return;
+ } catch (HelixException e) {
+ if (attempt >= DELETE_JOB_MAX_ATTEMPTS) {
+ throw e;
+ }
+ long backoffMs = DELETE_JOB_BACKOFF_MS * attempt;
Review Comment:
Nit: worst case this blocks the caller (an API request thread) ~3s (1s + 2s)
before rethrowing. Fine for this path, just noting the added latency is now
user-visible on a doomed delete.
--
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]
