Skip to content

[fix][txn] Catch and log runtime exceptions in async operations#19258

Merged
nicoloboschi merged 3 commits intoapache:masterfrom
nicoloboschi:transaction-catch-sync-err
Jan 18, 2023
Merged

[fix][txn] Catch and log runtime exceptions in async operations#19258
nicoloboschi merged 3 commits intoapache:masterfrom
nicoloboschi:transaction-catch-sync-err

Conversation

@nicoloboschi
Copy link
Copy Markdown
Contributor

Motivation

In the transaction coordinator code, most of the async execution doesn't properly runtime exceptions that might happens.
The exception is not logged and the CompletableFuture is not completed.
Similar changes in past #17484

Modifications

  • New utility method FutureUtil#safeRunAsync(Runnable runnable, Executor executor, CompletableFuture completableFuture) that completes exceptionally the completablefuture if an error has been caught in the runnable
  • Added in MLTransactionMetadataStore and TxnLogBufferedWriter

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 17, 2023
Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java Outdated
Copy link
Copy Markdown
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@nicoloboschi nicoloboschi self-assigned this Jan 18, 2023
@nicoloboschi nicoloboschi added this to the 2.12.0 milestone Jan 18, 2023
@nicoloboschi
Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 18, 2023

Codecov Report

Merging #19258 (5327fc2) into master (299bd70) will increase coverage by 3.01%.
The diff coverage is 81.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19258      +/-   ##
============================================
+ Coverage     46.37%   49.39%   +3.01%     
- Complexity    18221    19243    +1022     
============================================
  Files          1599     1609      +10     
  Lines        130054   126294    -3760     
  Branches      14317    13728     -589     
============================================
+ Hits          60310    62378    +2068     
+ Misses        63630    57387    -6243     
- Partials       6114     6529     +415     
Flag Coverage Δ
inttests 22.28% <51.31%> (?)
systests 19.51% <0.00%> (?)
unittests 45.18% <76.31%> (-1.19%) ⬇️

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

Impacted Files Coverage Δ
...java/org/apache/pulsar/common/util/FutureUtil.java 54.32% <66.66%> (+6.32%) ⬆️
...saction/coordinator/impl/TxnLogBufferedWriter.java 81.85% <80.76%> (+0.84%) ⬆️
...n/coordinator/impl/MLTransactionMetadataStore.java 67.09% <84.09%> (+0.32%) ⬆️
...pache/pulsar/client/impl/CompactionReaderImpl.java 0.00% <0.00%> (-90.00%) ⬇️
...g/apache/pulsar/client/impl/RawBatchConverter.java 3.12% <0.00%> (-89.07%) ⬇️
.../pulsar/compaction/StrategicTwoPhaseCompactor.java 0.00% <0.00%> (-76.20%) ⬇️
...lsar/client/impl/RawBatchMessageContainerImpl.java 0.00% <0.00%> (-73.14%) ⬇️
...ava/org/apache/pulsar/io/core/BatchPushSource.java 0.00% <0.00%> (-61.54%) ⬇️
...lsar/broker/service/StickyKeyConsumerSelector.java 50.00% <0.00%> (-50.00%) ⬇️
...ion/buffer/metadata/TransactionBufferSnapshot.java 42.85% <0.00%> (-42.86%) ⬇️
... and 828 more

@nicoloboschi nicoloboschi merged commit cfd7e60 into apache:master Jan 18, 2023
liangyepianzhou pushed a commit that referenced this pull request Feb 7, 2023
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
@thetumbled

This comment was marked as resolved.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants