[fix] [ml] Fix orphan scheduled task for ledger create timeout check #21542

poorbarcode · 2023-11-08T08:48:06Z

Motivation

When an ML tries to create a new ledger, it will create a delay task to check if the ledger create request is timeout^[1].

However, we should cancel this delay task after the request to create new ledgers is finished. Otherwise, these tasks will cost unnecessary CPU resources^[2].

[1]: https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L4070-L4082

bookKeeper.asyncCreateLedger(config.getEnsembleSize(), config.getWriteQuorumSize(), config.getAckQuorumSize(), digestType, config.getPassword(), cb, ledgerCreated, finalMetadata);

scheduledExecutor.schedule(() -> {
    if (!ledgerCreated.get()) {
        cb.createComplete(BKException.Code.TimeoutException, null, ledgerCreated);
    }
}, config.getMetadataOperationsTimeoutSeconds(), TimeUnit.SECONDS);

[2]: Thousands delay tasks in memory

Modifications

Cancel the scheduled task after the create ledger request is finished

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: x

codelipenghui

LGTM! and Good Job!
Just left a minor comment.

codelipenghui · 2023-11-10T08:18:04Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

            } else {
                if (log.isDebugEnabled()) {
                    log.debug("[{}] Ledger already created when timeout task is triggered", name);
                }
            }
        }, config.getMetadataOperationsTimeoutSeconds(), TimeUnit.SECONDS);
+
+        ledgerFutureHook.whenComplete((ignore, ex) -> {
+            if (!timeoutChecker.isDone()) {


From the API description

/** * Attempts to cancel execution of this task. This method has no * effect if the task is already completed or cancelled, or could * not be cancelled for some other reason. Otherwise, if this * task has not started when {@code cancel} is called, this task * should never run. If the task has already started, then the * {@code mayInterruptIfRunning} parameter determines whether the * thread executing this task (when known by the implementation) * is interrupted in an attempt to stop the task.

It looks like we don't need this check.

codecov-commenter · 2023-11-10T22:04:36Z

Codecov Report

Merging #21542 (4c2477c) into master (44abba9) will decrease coverage by 0.07%.
Report is 9 commits behind head on master.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master   #21542      +/-   ##
============================================
- Coverage     73.27%   73.20%   -0.07%     
+ Complexity    32676    32570     -106     
============================================
  Files          1892     1892              
  Lines        140632   140720      +88     
  Branches      15467    15479      +12     
============================================
- Hits         103044   103018      -26     
- Misses        29497    29588      +91     
- Partials       8091     8114      +23

Flag	Coverage Δ
inttests	`24.12% <42.85%> (-0.22%)`	⬇️
systests	`24.70% <42.85%> (-0.28%)`	⬇️
unittests	`72.50% <50.00%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	`80.64% <50.00%> (-0.39%)`	⬇️

... and 100 files with indirect coverage changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

mattisonchao

Question:

What is the benefit of using CompletableFuture? Because it has an atomic cancel method, why not pass timeoutChecker directly?

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

…21542) ### Motivation When an ML tries to create a new ledger, it will create a delay task to check if the ledger create request is timeout[1]. However, we should cancel this delay task after the request to create new ledgers is finished. Otherwise, these tasks will cost unnecessary CPU resources. ### Modifications Cancel the scheduled task after the create ledger request is finished

…pache#21542) ### Motivation When an ML tries to create a new ledger, it will create a delay task to check if the ledger create request is timeout[1]. However, we should cancel this delay task after the request to create new ledgers is finished. Otherwise, these tasks will cost unnecessary CPU resources. ### Modifications Cancel the scheduled task after the create ledger request is finished

…21542) When an ML tries to create a new ledger, it will create a delay task to check if the ledger create request is timeout[1]. However, we should cancel this delay task after the request to create new ledgers is finished. Otherwise, these tasks will cost unnecessary CPU resources. Cancel the scheduled task after the create ledger request is finished (cherry picked from commit e7536a2)

…pache#21542) ### Motivation When an ML tries to create a new ledger, it will create a delay task to check if the ledger create request is timeout[1]. However, we should cancel this delay task after the request to create new ledgers is finished. Otherwise, these tasks will cost unnecessary CPU resources. ### Modifications Cancel the scheduled task after the create ledger request is finished

…pache#21542) When an ML tries to create a new ledger, it will create a delay task to check if the ledger create request is timeout[1]. However, we should cancel this delay task after the request to create new ledgers is finished. Otherwise, these tasks will cost unnecessary CPU resources. Cancel the scheduled task after the create ledger request is finished (cherry picked from commit e7536a2)

poorbarcode added 3 commits November 8, 2023 15:16

-

7cc7c73

[fix] [ml] Fix orphan timeout check tasks after ML closed

f5343e9

add comment for the test

d9d8803

poorbarcode requested review from lhotari, hangc0276, Technoboy-, codelipenghui and mattisonchao November 8, 2023 08:48

poorbarcode self-assigned this Nov 8, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 8, 2023

poorbarcode added release/2.10.6 category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost release/3.0.3 release/2.11.4 and removed doc-not-needed Your PR changes do not impact docs labels Nov 8, 2023

poorbarcode added this to the 3.2.0 milestone Nov 8, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 8, 2023

poorbarcode changed the title ~~[fix] [ml] Fix orphan task for ledger create timeout check~~ [fix] [ml] Fix orphan scheduled task for ledger create timeout check Nov 8, 2023

codelipenghui approved these changes Nov 10, 2023

View reviewed changes

address comment

04bb569

poorbarcode added the ready-to-test label Nov 10, 2023

Technoboy- approved these changes Nov 10, 2023

View reviewed changes

address comment

4c2477c

mattisonchao reviewed Nov 11, 2023

View reviewed changes