Skip to content

[Fix-18692][Master] Fix ignored dependent task check interval - #18694

Open
Mrhs121 wants to merge 3 commits into
apache:devfrom
Mrhs121:fix-18692
Open

Mrhs121 wants to merge 3 commits into
apache:devfrom
Mrhs121:fix-18692

Conversation

@Mrhs121

@Mrhs121 Mrhs121 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Was this PR generated or assisted by AI?

Yes, I design the architecture and write the core code myself, then use an LLM to review and optimize the logic.

Purpose of the pull request

fix #18692

Brief change log

Verify this pull request

20s config
截屏2026-10-10 16 23 37

10s config
截屏2026-10-10 16 22 53

(or)

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

@SbloodyS SbloodyS added the bug Something isn't working label Oct 12, 2026
@SbloodyS SbloodyS added this to the 3.5.0 milestone Oct 12, 2026
SbloodyS
SbloodyS previously approved these changes Oct 12, 2026

@SbloodyS SbloodyS left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ruanwenjun ruanwenjun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some nit

@Slf4j
public class LogicTaskExecutor extends AbstractTaskExecutor {

private static final long MIN_STATE_TRACK_INTERVAL_MILLIS = TimeUnit.SECONDS.toMillis(1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final long MIN_STATE_TRACK_INTERVAL_MILLIS = TimeUnit.SECONDS.toMillis(1);
private static final long MIN_STATE_TRACK_INTERVAL_MILLIS = TimeUnit.SECONDS.toMillis(10);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// if the status is initialized, then we can directly change to paused
if (logicTask != null) {
logicTask.pause();
resetStateTrackTime();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's best not to reset; I'm worried it might cause some bugs involving infinite loops.

@Mrhs121 Mrhs121 Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I reviewed the logic and didn't find a path where the reset itself would cause an infinite loop. However, I did identify a concurrency issue between state checks and pause/kill operations.

An immediate state check is still needed after pause/kill: these operations update the dependent task's plugin state, but the executor reports that state during its next check. Once the configured checkInterval is respected, a value of 900s could otherwise delay pause/kill status reporting by almost 15 minutes.

I've replaced resetStateTrackTime() with a one-time request using AtomicBoolean. Only the tracking thread now updates latestStateTrackTime. It consumes the request with getAndSet(false) before polling, so a request arriving during a check remains pending for the next iteration. Without another request, polling resumes at the configured interval; paused/killed tasks are removed from active tracking. A single request does not keep triggering checks.

I also found an existing race in DependentLogicTask: a dependency query could return after pause/kill and overwrite the paused/killed state. State reads and writes now use short synchronized blocks, and query results are applied only if the task is still running. The database query stays outside the lock, so pause/kill does not have to wait for it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under the current changes, there is still a possibility of long wait times, depending on when getTaskExecutionStateCheckInterval is called.

To better handle this issue, a better approach might be to use a fixed polling interval at the framework level to check the TaskExecutor’s status, and then, within the LogicTask, use the interval specified in the task parameters to determine whether to trigger the underlying query or simply return the cached status.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, makes sense. I’d adjust the implementation as follows:

  • Keep framework-level state polling at a fixed 10-second interval
  • Move the checkInterval handling into DependentLogicTask. The first check would query dependencies immediately; subsequent checks would return the cached status until the configured interval has elapsed. For example, with checkInterval = 20s, the check at 10s would return the cached status, and the check at 20s would query dependencies again
  • Check for paused, killed, or completed states before applying the query interval. This allows pause/kill to be reported on the next framework poll without waiting for checkInterval, so the extra one-time state-check request can be removed
  • Keep the concurrency protection: use short synchronized blocks for state access, leave the dependency query outside the lock, and apply its result only if the task is still running. This prevents a query from overwriting a concurrent pause/kill state without making the control operation wait for the query

Does this approach look good to you?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend bug Something isn't working test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [Module Name] Bug title V3.3.2 dependent component Check interval invalidate

3 participants