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

[improve][txn] PIP-160: Transaction buffered writer supports Timer #16727

Merged
merged 7 commits into from
Jul 26, 2022

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jul 21, 2022

Master Issue: #15370

Motivation

see #15370

Modifications

In the original design of the transaction buffered writer, we use a scheduled executor to execute timed tasks. There are no scheduled executors that match this scenario, and It's a little expensive to have a new scheduled executor with 1 millis tick time.
The PulsarService.brokerClientSharedTimer is very match to this scenario, so we should reuse it instead to create a new scheduled executor.

So we should make transaction buffered writer supports io.netty.Timer.

Documentation

  • doc-required

  • doc-not-needed

  • doc

  • doc-complete

@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 22, 2022
@congbobo184 congbobo184 added area/transaction type/feature The PR added a new feature or issue requested a new feature labels Jul 22, 2022
@congbobo184 congbobo184 added this to the 2.11.0 milestone Jul 22, 2022
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@poorbarcode poorbarcode changed the title [improve][txn] PIP-160: Transaction buffered writer supports both ScheduledService and Timer [improve][txn] PIP-160: Transaction buffered writer supports Timer Jul 22, 2022
@@ -73,15 +74,15 @@

private final ManagedLedger managedLedger;

private final ScheduledExecutorService scheduledExecutorService;
private Timer timer;
Copy link
Contributor

Choose a reason for hiding this comment

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

final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already fixed.

@poorbarcode poorbarcode requested a review from congbobo184 July 22, 2022 08:11
Copy link
Member

@coderzc coderzc left a comment

Choose a reason for hiding this comment

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

LGTM

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 type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants