Skip to content

[feat][txn] implement the SnapshotSegmentAbortedTxnProcessor - #18273

Merged
congbobo184 merged 42 commits into
apache:masterfrom
liangyepianzhou:xiangying/pip/snapshotsegmentAbortedProcessor
Feb 13, 2023
Merged

[feat][txn] implement the SnapshotSegmentAbortedTxnProcessor#18273
congbobo184 merged 42 commits into
apache:masterfrom
liangyepianzhou:xiangying/pip/snapshotsegmentAbortedProcessor

Conversation

@liangyepianzhou

@liangyepianzhou liangyepianzhou commented Nov 1, 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.

Add persistentWorker to handle snapshot persistenting
image
The first four items below are the corresponding four tasks in the figure. The fifth item is not strictly a task, but a part of the first two tasks.

  • takeSnapshotSegmentAsync -> writeSnapshotSegmentAsync

    • These two method is used to persist the snapshot segment.
  • deleteSnapshotSegment

    • This method is used to delete the snapshot segment.
  • clearSnapshotSegmentAndIndexes

    • Delete all segments and then delete the index of this topic.
  • updateSnapshotIndex

    • Called by the deleteSnapshotSegment and writeSnapshotSegmentAsync. Do update the index after writing the snapshot segment.
    • Called to update index snapshot by takeSnapshotByChangeTimes and takeSnapshotByTimeout.
    • Called by recovery as a compensation mechanism for updating the index.

Documentation

  • doc-not-needed
    (Please explain why)

Matching PR in the forked repository

PR in forked repository: liangyepianzhou#9

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Nov 1, 2022

@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.

Have not read this PR completely, left some comments

@github-actions

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@congbobo184 congbobo184 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.

reviewed a part,

@liangyepianzhou

Copy link
Copy Markdown
Contributor Author

Create a delete task for each segment, and update the index within each delete task. This method costs too much.

In factly, this logic in clearSnapshotSegmentAndIndexes is executed in a single task.
There does not open new tasks for each segment deleting and index updating.

@github-actions github-actions Bot removed the Stale label Dec 30, 2022
@codecov-commenter

codecov-commenter commented Feb 3, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 3.05556% with 349 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.04%. Comparing base (492a9c3) to head (c7c1294).
⚠️ Report is 3058 commits behind head on master.

Files with missing lines Patch % Lines
...r/impl/SnapshotSegmentAbortedTxnProcessorImpl.java 0.00% 336 Missing ⚠️
...er/metadata/v2/TransactionBufferSnapshotIndex.java 0.00% 4 Missing ⚠️
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 50.00% 3 Missing ⚠️
...ransaction/buffer/impl/TopicTransactionBuffer.java 57.14% 1 Missing and 2 partials ⚠️
.../metadata/v2/TransactionBufferSnapshotSegment.java 0.00% 2 Missing ⚠️
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #18273       +/-   ##
=============================================
+ Coverage     47.46%   61.04%   +13.58%     
- Complexity    10727    25813    +15086     
=============================================
  Files           711     1910     +1199     
  Lines         69456   138905    +69449     
  Branches       7452    15241     +7789     
=============================================
+ Hits          32964    84798    +51834     
- Misses        32810    46331    +13521     
- Partials       3682     7776     +4094     
Flag Coverage Δ
inttests 24.75% <1.94%> (?)
systests 24.55% <0.00%> (?)
unittests 58.91% <3.05%> (+11.44%) ⬆️

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

Files with missing lines Coverage Δ
...r/service/SystemTopicTxnBufferSnapshotService.java 78.26% <100.00%> (ø)
...er/systopic/NamespaceEventsSystemTopicFactory.java 71.42% <100.00%> (-1.91%) ⬇️
...ransactionBufferSnapshotBaseSystemTopicClient.java 68.42% <100.00%> (+0.42%) ⬆️
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% <0.00%> (ø)
.../metadata/v2/TransactionBufferSnapshotSegment.java 0.00% <0.00%> (ø)
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 68.08% <50.00%> (-2.57%) ⬇️
...ransaction/buffer/impl/TopicTransactionBuffer.java 58.67% <57.14%> (+0.53%) ⬆️
...er/metadata/v2/TransactionBufferSnapshotIndex.java 0.00% <0.00%> (ø)
...r/impl/SnapshotSegmentAbortedTxnProcessorImpl.java 0.00% <0.00%> (ø)

... and 1526 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.

@congbobo184
congbobo184 merged commit f5c532d into apache:master Feb 13, 2023
@lhotari

lhotari commented Feb 27, 2023

Copy link
Copy Markdown
Member

This PR introduced a new flaky test #19650, please fix

@lhotari

lhotari commented Feb 26, 2024

Copy link
Copy Markdown
Member

the implementation contains multiple thread-safety issues, I filed #22116

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

Labels

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