xiangfu0 commented on code in PR #20009:
URL: https://github.com/apache/pinot/pull/20009#discussion_r4368564533
##########
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:
Confirmed in Helix 2.0.1: a non-forced `deleteJob` throws
`IllegalArgumentException("JobQueue ... config is not found!")` once the queue
config is gone. The forced path never reads the queue config, so forced retries
are unaffected. Leaving this as is: if a forced `deleteTaskQueue` lands during
the backoff, the job is removed along with the queue, so that error describes
the actual state. The same error already happens without this change when a
queue delete runs just before a non-forced task delete, so the retry does not
add a new failure mode.
##########
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:
Yes, that is intended. The base backoff was raised from 100 ms to 1 s on
purpose, to give the Helix task pipeline time to finish writing the job
context. The wait only happens after an attempt fails; a delete that succeeds
on the first try returns as before. On the table-delete path it replaces an
immediate 500 that the client would otherwise have to retry.
--
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]
