[enh][transaction] Optimize to reuse transaction buffer snapshot writer by gaoran10 · Pull Request #15015 · apache/pulsar

gaoran10 · 2022-04-03T17:56:38Z

Motivation

The transaction buffer(short for TB) snapshot topic is used to persistent snapshot data for TB, the snapshot can reduce read data when recovering.

Currently, if enable the transaction feature, every topic will create a TB snapshot producer, the topic of the producer is a namespace system topic, so we don't need to create a producer for every topic, one producer per namespace is enough.

Modification

Add a new inner class ReferenceCountedWriter, if the ReferenceCountedWriter for one namespace does not exist, it will be initialized, or else its reference count will be increased, if the reference count reduces to 0, it will be removed from map-cache.

Verifying this change

Adjust existing tests to cover this change.

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): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

mattisonchao · 2022-04-06T09:27:42Z

+
+        private synchronized void initWriterFuture() {
+            this.future = service.getTransactionBufferSystemTopicClient(namespaceName).newWriterAsync();
+            this.future.thenRunAsync(this.backoff::reset).exceptionally(throwable -> {


Why use thenRunAsync with common-pool here? I'm not sure we should use a specific executor?

Does the common pool have some potential problems?

It's OK here.
Just to prevent others from using common-pool to do some IO-intensive tasks and affecting there.
After deep thinking, I think we need to check all abuses of common-pool, not against it.

mattisonchao · 2022-04-06T12:04:48Z

+                // Resolve potential race condition problem, if retain method encounter reference count exception
+                // or other exceptions, create a new `ReferenceCountedWriter`, when the `ReferenceCountedWriter` release
+                // but didn't remove from `writerFutureMap`.
+                return new ReferenceCountedWriter(namespaceName, this);


Question: Should we close the original writer?

The previous writer will be closed when its reference count reduces to 0, if its reference count reduces to 0, it must call the method deallocate.

Ok, thanks for your explanation.

congbobo184 · 2022-04-07T03:06:15Z

+
+    // The class ReferenceCountedWriter will maintain the reference count,
+    // when the reference count decrement to 0, it will be removed from writerFutureMap, the writer will be closed.
+    public static class ReferenceCountedWriter extends AbstractReferenceCounted {


I suggest don't extends AbstractReferenceCounted, deallocate seem not be used. and is complicated to implement

When the reference count reduces to 0, the deallocate will be called, maybe using it to compute the reference count is simple, it's thread-safe.

congbobo184 · 2022-04-07T03:07:51Z

+        private synchronized void initWriterFuture() {
+            this.future = service.getTransactionBufferSystemTopicClient(namespaceName).newWriterAsync();
+            this.future.thenRunAsync(this.backoff::reset).exceptionally(throwable -> {
+                long delay = backoff.next();


seem we only use backoff.next() once and then reset

If encounter exceptions the backoff will increase, if the writer creates successfully, it will be reset.

The transaction buffer(short for TB) snapshot topic is used to persistent snapshot data for TB, the snapshot can reduce read data when recovering. Currently, if enable transaction feature, every topic will create a TB snapshot producer, the topic of the producer is a namespace system topic, so we don't need to create producer for every topic, one producer per namespace is enough. # Modification Add a new inner class `ReferenceCountedWriter`, it extends `AbstractReferenceCounted`, if the `ReferenceCountedWriter` is not exist, it will be initilied, or else its reference count will be increased, after the reference count reduce to 0, it will be removed from map cache.

…writer

congbobo184

Good work! left some comments

eolivelli · 2022-04-24T16:10:03Z

    }
+
+    private void closePendingCloseWriter() {
+        Iterator<Writer<TransactionBufferSnapshot>> iterator = pendingCloseWriterList.stream().iterator();


Nit: why are you creating the stream?
We can iterate on the LinkedList directly, creating less garbage and also making the code simpler

github-actions · 2022-05-25T02:14:47Z

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

github-actions · 2022-06-24T02:17:12Z

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

gaoran10 · 2023-02-28T07:49:53Z

This PR is too old, I create a new one #19641.

github-actions Bot assigned gaoran10 Apr 3, 2022

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 3, 2022

gaoran10 requested review from 315157973, Technoboy-, codelipenghui, congbobo184, eolivelli, hangc0276, merlimat and zymap April 3, 2022 17:58

gaoran10 added type/feature The PR added a new feature or issue requested a new feature area/transaction labels Apr 3, 2022

Technoboy- reviewed Apr 4, 2022

View reviewed changes

Comment thread .../src/main/java/org/apache/pulsar/broker/service/SystemTopicBaseTxnBufferSnapshotService.java Outdated

gaoran10 changed the title ~~[enh][transaction] Optimize to reuse transaction buffer snapshot writer~~ [WIP][enh][transaction] Optimize to reuse transaction buffer snapshot writer Apr 4, 2022

gaoran10 changed the title ~~[WIP][enh][transaction] Optimize to reuse transaction buffer snapshot writer~~ [enh][transaction] Optimize to reuse transaction buffer snapshot writer Apr 6, 2022

liangyepianzhou approved these changes Apr 6, 2022

View reviewed changes

mattisonchao reviewed Apr 6, 2022

View reviewed changes

congbobo184 reviewed Apr 7, 2022

View reviewed changes

mattisonchao approved these changes Apr 7, 2022

View reviewed changes

gaoran10 changed the title ~~[enh][transaction] Optimize to reuse transaction buffer snapshot writer~~ [WIP][enh][transaction] Optimize to reuse transaction buffer snapshot writer Apr 8, 2022

gaoran10 changed the title ~~[WIP][enh][transaction] Optimize to reuse transaction buffer snapshot writer~~ [enh][transaction] Optimize to reuse transaction buffer snapshot writer Apr 12, 2022

gaoran10 added 5 commits April 13, 2022 00:00

avoid remove new added reference count object

edf2f6b

fix check style

9a500e5

remove backoff operation for re-creating transaction buffer snapshot …

acb795f

…writer

Adjust the reference counted writer

7cac709

gaoran10 force-pushed the tb-snapshot-writer-reuse branch from a3c098b to 7cac709 Compare April 12, 2022 16:01

gaoran10 added 2 commits April 13, 2022 00:40

remove useless import

59e4ad4

fix thread safe problem

5889132

congbobo184 approved these changes Apr 14, 2022

View reviewed changes

congbobo184 reviewed Apr 14, 2022

View reviewed changes

Comment thread .../src/main/java/org/apache/pulsar/broker/service/SystemTopicBaseTxnBufferSnapshotService.java

address comments

3786a1a

congbobo184 approved these changes Apr 24, 2022

View reviewed changes

eolivelli requested changes Apr 24, 2022

View reviewed changes

github-actions Bot added the lifecycle/stale label May 25, 2022

tjiuming mentioned this pull request Jan 9, 2023

[fix][broker] Close transactionBuffer after MessageDeduplication#checkStatus failed #19157

Merged

4 tasks

tjiuming approved these changes Jan 9, 2023

View reviewed changes

gaoran10 closed this Feb 28, 2023

gaoran10 deleted the tb-snapshot-writer-reuse branch March 21, 2023 01:42

Conversation

gaoran10 commented Apr 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modification

Verifying this change

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

Documentation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoran10 Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaoran10 Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

congbobo184 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 25, 2022

Uh oh!

github-actions Bot commented Jun 24, 2022

Uh oh!

gaoran10 commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gaoran10 commented Apr 3, 2022 •

edited

Loading

gaoran10 Apr 7, 2022 •

edited

Loading

gaoran10 Apr 7, 2022 •

edited

Loading