Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat][broker] Segmented transaction buffer snapshot segment and index system topic #16931

Merged
merged 31 commits into from
Oct 24, 2022
Merged

[feat][broker] Segmented transaction buffer snapshot segment and index system topic #16931

merged 31 commits into from
Oct 24, 2022

Conversation

liangyepianzhou
Copy link
Contributor

@liangyepianzhou liangyepianzhou commented Aug 3, 2022

Master Issue: #16913

Motivation

Implement system topic client for snapshot segment topic and index topic to send segment snapshots or indexes.
The configuration transactionBufferSegmentedSnapshotEnabled is used in the Transaction Buffer to determine which AbortedTxnProcessor is adopted by this TB.

Modification

In the new implementation of the Transaction Buffer Snapshot System topic, because the system topic that needs to be processed has changed from the original one to three with different schemes, we have added generics to the TransactionBufferSnapshotBaseSystemTopicClient class and the SystemTopicTxnBufferSnapshotService class.
And Pulsar Service maintains a factory class TransactionBufferSnapshotServiceFactory used to obtain SystemTopicTxnBufferSnapshotService.
This way, we can obtain the required System topic client through pulsarService to read and send snapshots.
image

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/transaction labels Aug 8, 2022
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Aug 8, 2022
@liangyepianzhou liangyepianzhou changed the title [Improve][Txn]TransactionBuffer multiple-snapshot index topic [Improve][Txn][PIP-196]Segmented transaction buffer snapshot segment and index system topic Sep 6, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 6, 2022
@@ -93,22 +93,22 @@
* @return message id
* @throws PulsarClientException exception while write event cause
*/
MessageId write(T t) throws PulsarClientException;
MessageId write(T t, String key) throws PulsarClientException;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can retain write(T t), just add write(T t, String key)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, why mark this one as resolved, I don't see any conclusion for this one.
And it's better to use MessageId write(String key, T value).

Copy link
Contributor Author

@liangyepianzhou liangyepianzhou Oct 20, 2022

Choose a reason for hiding this comment

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

Because MessageId write(T t) throws PulsarClientException; can not be implement in the TransactionBufferSnapshotBaseSystemTopicClient.
We can not get the key from the value of type T.

@@ -93,22 +93,22 @@
* @return message id
* @throws PulsarClientException exception while write event cause
*/
MessageId write(T t) throws PulsarClientException;
MessageId write(T t, String key) throws PulsarClientException;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, why mark this one as resolved, I don't see any conclusion for this one.
And it's better to use MessageId write(String key, T value).

@codelipenghui
Copy link
Contributor

@poorbarcode @congbobo184 Please help review the PR again, thanks.

@codelipenghui codelipenghui changed the title [Improve][Txn][PIP-196]Segmented transaction buffer snapshot segment and index system topic [feat][broker] Segmented transaction buffer snapshot segment and index system topic Oct 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2022

Codecov Report

Merging #16931 (39da371) into master (6c65ca0) will increase coverage by 10.99%.
The diff coverage is 60.42%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #16931       +/-   ##
=============================================
+ Coverage     34.91%   45.91%   +10.99%     
- Complexity     5707    17672    +11965     
=============================================
  Files           607     1579      +972     
  Lines         53396   128705    +75309     
  Branches       5712    14161     +8449     
=============================================
+ Hits          18644    59093    +40449     
- Misses        32119    63498    +31379     
- Partials       2633     6114     +3481     
Flag Coverage Δ
unittests 45.91% <60.42%> (+10.99%) ⬆️

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

Impacted Files Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 52.06% <ø> (ø)
.../pulsar/broker/stats/BrokerOperabilityMetrics.java 98.21% <ø> (+5.56%) ⬆️
...ache/pulsar/broker/systopic/SystemTopicClient.java 0.00% <ø> (ø)
.../transaction/buffer/metadata/AbortTxnMetadata.java 28.57% <ø> (ø)
...ion/buffer/metadata/TransactionBufferSnapshot.java 42.85% <ø> (ø)
...er/metadata/v2/TransactionBufferSnapshotIndex.java 0.00% <0.00%> (ø)
.../metadata/v2/TransactionBufferSnapshotIndexes.java 0.00% <0.00%> (ø)
.../metadata/v2/TransactionBufferSnapshotSegment.java 0.00% <0.00%> (ø)
...oker/transaction/buffer/metadata/v2/TxnIDData.java 0.00% <0.00%> (ø)
...g/apache/pulsar/compaction/CompactedTopicImpl.java 69.28% <0.00%> (+58.57%) ⬆️
... and 1149 more

@congbobo184
Copy link
Contributor

congbobo184 commented Oct 24, 2022

please modify the PR description for the new commit, then can be merged. @liangyepianzhou

@liangyepianzhou liangyepianzhou merged commit d211766 into apache:master Oct 24, 2022
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 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants