Skip to content

[feat][txn] Implement the AbortedTxnProcessor for TransactionBuffer - #17847

Merged
congbobo184 merged 33 commits into
apache:masterfrom
liangyepianzhou:xiangying/pip196/aborted_txn_processor
Oct 31, 2022
Merged

[feat][txn] Implement the AbortedTxnProcessor for TransactionBuffer#17847
congbobo184 merged 33 commits into
apache:masterfrom
liangyepianzhou:xiangying/pip196/aborted_txn_processor

Conversation

@liangyepianzhou

@liangyepianzhou liangyepianzhou commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

Master Issue: #16913

Motivation

Implement an abortedTxnProcessor to handle the storage of the aborted transaction ID.

Modifications

The structure overview:
image

The main idea is to move the logic of the operation of checking and persistent aborted transaction IDs(take snapshots) and the operation of updating maxReadPosition into the AbortedTxnProcessor.
And the AbortedTxnProcessor can be implemented in different designs.

Documentation

  • doc-not-needed
    (Please explain why)

Matching PR in the forked repository

PR in forked repository: liangyepianzhou#7

@liangyepianzhou
liangyepianzhou marked this pull request as draft September 27, 2022 03:56
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 27, 2022
@liangyepianzhou liangyepianzhou changed the title [improve][txn] Implement the AbortedTxnProcessor for TransactionBuffer [feat][txn] Implement the AbortedTxnProcessor for TransactionBuffer Oct 24, 2022
@liangyepianzhou
liangyepianzhou marked this pull request as ready for review October 25, 2022 03:58

@poorbarcode poorbarcode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't read the whole thing, leave some comments

CompletableFuture<Void> completableFuture = new CompletableFuture<>();
persistentWorker.appendTask(PersistentWorker.OperationType.Close,
() -> persistentWorker.clearSnapshotSegmentAndIndexes()
.thenRun(() -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest whenComplete instead thenRun & exceptionally

@poorbarcode poorbarcode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't read the whole thing, leave some comments

return abortTxnSegments.values().stream()
.anyMatch(list -> list.contains(txnID)) || unsealedAbortedTxnIdSegment.contains(txnID);
} else {
PositionImpl maxReadPosition = abortTxnSegments.ceilingKey((PositionImpl) readPosition);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it does not exist in the segment, we directly check unsealed segment is ok

@congbobo184 congbobo184 reopened this Oct 31, 2022
@codecov-commenter

codecov-commenter commented Oct 31, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 45.33333% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.49%. Comparing base (0866c3a) to head (7b1b817).
⚠️ Report is 3402 commits behind head on master.

Files with missing lines Patch % Lines
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 51.19% 38 Missing and 3 partials ⚠️
...ransaction/buffer/impl/TopicTransactionBuffer.java 41.07% 30 Missing and 3 partials ⚠️
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% 6 Missing ⚠️
.../pulsar/broker/service/AbstractBaseDispatcher.java 50.00% 0 Missing and 1 partial ⚠️
.../metadata/v2/TransactionBufferSnapshotIndexes.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #17847       +/-   ##
=============================================
+ Coverage     38.97%   49.49%   +10.52%     
+ Complexity     8311     7002     -1309     
=============================================
  Files           683      400      -283     
  Lines         67325    43612    -23713     
  Branches       7217     4477     -2740     
=============================================
- Hits          26239    21586     -4653     
+ Misses        38079    19624    -18455     
+ Partials       3007     2402      -605     
Flag Coverage Δ
unittests 49.49% <45.33%> (+10.52%) ⬆️

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

Files with missing lines Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java 56.13% <100.00%> (-0.85%) ⬇️
...ransaction/buffer/impl/InMemTransactionBuffer.java 0.00% <ø> (ø)
...nsaction/buffer/impl/TransactionBufferDisable.java 52.63% <ø> (ø)
.../pulsar/broker/service/AbstractBaseDispatcher.java 52.43% <50.00%> (+0.29%) ⬆️
.../metadata/v2/TransactionBufferSnapshotIndexes.java 0.00% <0.00%> (ø)
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% <0.00%> (ø)
...ransaction/buffer/impl/TopicTransactionBuffer.java 44.18% <41.07%> (-0.21%) ⬇️
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 51.19% <51.19%> (ø)

... and 313 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

area/transaction doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants