[Transaction] Transaction coordinator fence mechanism. #11357

congbobo184 · 2021-07-19T03:26:41Z

Verifying this change

Add the tests for it

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: (yes)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

eolivelli · 2021-07-19T05:56:05Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/TransactionMetadataStoreService.java

+                        stores.put(tcId, store);
+                        LOG.info("Added new transaction meta store {}", tcId);
+                        while (true) {
+                            CompletableFuture<Void> future = deque.poll();


This is a busy loop.
Can we add a timeout to poll? This way the thread may suspend

eolivelli · 2021-07-19T05:56:25Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/TransactionMetadataStoreService.java

+                        tcLoadSemaphore.release();
+
+                        while (true) {
+                            CompletableFuture<Void> future = deque.poll();


The same here

pulsar-broker/src/main/java/org/apache/pulsar/broker/TransactionMetadataStoreService.java

eolivelli · 2021-07-19T05:58:24Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/TransactionMetadataStoreService.java

                || e instanceof RequestTimeoutException
                || e instanceof ManagedLedgerException
                || e instanceof BrokerPersistenceException
                || e instanceof LookupException
                || e instanceof ReachMaxPendingOpsException
-                || e instanceof ConnectException;
+                || e instanceof ConnectException)
+                && !(e instanceof ManagedLedgerException.ManagedLedgerFencedException);


How is it possible that an object is an instance of the list of classes above and of this class?

it is ManagedLedgerException.ManagedLedgerNotFoundException and many more

we should add some "isRetryable" method to ManagedLedgerException
otherwise this code will be hard to be maintained

cc @merlimat

eolivelli · 2021-07-19T05:59:35Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

@@ -1675,6 +1677,17 @@ public boolean isTopicNsOwnedByBroker(TopicName topicName) {
                                : CompletableFuture.completedFuture(null)));
            }
        });
+        if (getPulsar().getConfig().isTransactionCoordinatorEnabled()
+                && serviceUnit.getNamespaceObject().toString().equals(NamespaceName.SYSTEM_NAMESPACE.toString())) {


It is better to not compare on the result of toString.
It may result to unpredictable results.
It is better to add an explicit method to verify the equality

eolivelli · 2021-07-19T06:00:32Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

        final long requestId = command.getRequestId();
        if (log.isDebugEnabled()) {
            command.getPartitionsList().forEach(partion ->
                    log.debug("Receive add published partition to txn request {} "
                            + "from {} with txnId {}, topic: [{}]", requestId, remoteAddress, txnID, partion));
        }
+
+        if (!service.getPulsar().getConfig().isTransactionCoordinatorEnabled()) {
+            BrokerServiceException.NotAllowedException ex =


Can we create a method for this code that is repeated more times

Anonymitaet · 2021-07-20T03:00:36Z

Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

…mechanism

eolivelli

Are Pulsar 2.8.0 clients able to connect to a 2.9.0 if they enable transactions ?
it looks like there is a wire protocol change (tcClientConnect).

I am aware of projects that are starting to use transactions, as we declared them "ready" in 2.8.0.

We should not break compatibility with 2.8 clients

eolivelli · 2021-09-14T11:27:34Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/TransactionMetadataStoreService.java

                || e instanceof RequestTimeoutException
                || e instanceof ManagedLedgerException
                || e instanceof BrokerPersistenceException
                || e instanceof LookupException
                || e instanceof ReachMaxPendingOpsException
-                || e instanceof ConnectException;
+                || e instanceof ConnectException)
+                && !(e instanceof ManagedLedgerException.ManagedLedgerFencedException);


we should add some "isRetryable" method to ManagedLedgerException
otherwise this code will be hard to be maintained

cc @merlimat

eolivelli · 2021-09-14T11:27:59Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

@@ -691,7 +692,11 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
        }
        // If the topic name is a partition name, no need to get partition topic metadata again
        if (topicName.isPartitioned()) {
-            internalUnloadNonPartitionedTopic(asyncResponse, authoritative);
+            if (topicName.toString().startsWith(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString())) {


we should have a better way to detect a system topic, IIRC we added something about this

eolivelli · 2021-09-14T11:28:37Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

+                && serviceUnit.getNamespaceObject().equals(NamespaceName.SYSTEM_NAMESPACE)) {
+            TransactionMetadataStoreService metadataStoreService =
+                    this.getPulsar().getTransactionMetadataStoreService();
+            // if have store belongs to this bundle, remove and close the store


typo "if the store" and not "if have store"

eolivelli · 2021-09-14T11:29:40Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java

@@ -396,6 +396,7 @@ private static void verifyCoordinatorStats(String state,
    private void initTransaction(int coordinatorSize) throws Exception {
        admin.topics().createPartitionedTopic(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString(), coordinatorSize);
        admin.lookups().lookupTopic(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString());
+        pulsarClient = PulsarClient.builder().serviceUrl(lookupUrl.toString()).enableTransaction(true).build();


please close this client, otherwise the new variable below will override the reference and we will leak a PulsarClient

eolivelli · 2021-09-14T11:29:53Z

...oker/src/test/java/org/apache/pulsar/broker/service/TransactionMetadataStoreServiceTest.java

@@ -66,11 +75,17 @@ protected void cleanup() throws Exception {
    }

    @Test
-    public void testAddAndRemoveTransactionMetadataStore() {
+    public void testCloseLock() {


remove empty test case ?

…mechanism

eolivelli

LGTM

…mand. (#12301) ## Motivation now #11357 has merged, #11357 use `SUCCESS` command handle tcClientConnectRequest, This is not conducive to later expansion. So add a individual response for `CommandTcClientConnectRequest` ## implement add command ``` message CommandTcClientConnectResponse { required uint64 request_id = 1; optional ServerError error = 2; optional string message = 3; } ``` In order to ensure that the new client is compatible with the old broker, I update protocol version to 19. ``` v19 = 19; // Add CommandTcClientConnectRequest and CommandTcClientConnectResponse ``` if broker protocol version > 18 we should send TcClientConnectCommand if broker protocol version <= 18 we don't need to send TcClientConnectCommand

…mand. (apache#12301) ## Motivation now apache#11357 has merged, apache#11357 use `SUCCESS` command handle tcClientConnectRequest, This is not conducive to later expansion. So add a individual response for `CommandTcClientConnectRequest` ## implement add command ``` message CommandTcClientConnectResponse { required uint64 request_id = 1; optional ServerError error = 2; optional string message = 3; } ``` In order to ensure that the new client is compatible with the old broker, I update protocol version to 19. ``` v19 = 19; // Add CommandTcClientConnectRequest and CommandTcClientConnectResponse ``` if broker protocol version > 18 we should send TcClientConnectCommand if broker protocol version <= 18 we don't need to send TcClientConnectCommand

[Transaction] Transaction coordinator fence mechanism.

a30ec4b

congbobo184 requested review from merlimat, eolivelli, codelipenghui and gaoran10 July 19, 2021 03:26

congbobo184 self-assigned this Jul 19, 2021

Add admin unload tc logical

4438bb7

eolivelli reviewed Jul 19, 2021

View reviewed changes

congbobo184 added the doc-not-needed Your PR changes do not impact docs label Jul 23, 2021

codelipenghui mentioned this pull request Jul 23, 2021

Add getTxnID method in Transaction.java #11438

Merged

congbobo184 mentioned this pull request Aug 11, 2021

Transaction coordinator onLoad and unLoad problem. streamnative/pulsar-archived#2563

Closed

congbo added 7 commits August 13, 2021 18:55

Merge branch 'master' into congbobo184_transaction_coordinator_fence_…

46a66ab

…mechanism

Merge branch 'master' into congbobo184_transaction_coordinator_fence_…

2a1d84f

…mechanism

accept some comments

f259537

Fix some code style

d56a3ec

Fix some code style

013ce9d

Add cpp tc client connnect command

bb5c25f

Fix cpp command

3fafa94

eolivelli requested changes Sep 14, 2021

View reviewed changes

eolivelli added area/broker component/protocol labels Sep 14, 2021

congbo added 4 commits September 22, 2021 19:28

recover listener logical in order to maintain compatibility

2b147a1

change code style

82f6d72

fix some comment

71f9000

Merge branch 'master' into congbobo184_transaction_coordinator_fence_…

c5dba54

…mechanism

eolivelli approved these changes Oct 8, 2021

View reviewed changes

eolivelli merged commit c14d079 into apache:master Oct 8, 2021

eolivelli added this to the 2.9.0 milestone Oct 8, 2021

eolivelli pushed a commit that referenced this pull request Oct 8, 2021

[Transaction] Transaction coordinator fence mechanism. (#11357)

f537426

congbobo184 mentioned this pull request Oct 8, 2021

[Transaction] Transaction coordinator client connect add response command. #12301

Merged

codelipenghui added release/2.8.2 and removed release/2.8.2 labels Oct 11, 2021

congbobo184 mentioned this pull request Oct 11, 2021

PIP 98: Transaction coordinator loading mechanism. #12129

Closed

congbobo184 mentioned this pull request Nov 17, 2021

pulsar transaction error #12829

Closed

bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022

[Transaction] Transaction coordinator fence mechanism. (apache#11357)

3e5966d

Technoboy- mentioned this pull request Apr 24, 2022

[cleanup][tx] Cleanup TransactionMetadataStoreService. #15286

Merged

1 task

Lanayx mentioned this pull request Nov 14, 2023

Add Transaction supported on 2.9+ fsprojects/pulsar-client-dotnet#237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transaction] Transaction coordinator fence mechanism. #11357

[Transaction] Transaction coordinator fence mechanism. #11357

congbobo184 commented Jul 19, 2021 •

edited

Loading

eolivelli Jul 19, 2021

eolivelli Jul 19, 2021

eolivelli Jul 19, 2021

congbobo184 Sep 2, 2021

eolivelli Sep 14, 2021

eolivelli Jul 19, 2021

eolivelli Jul 19, 2021

Anonymitaet commented Jul 20, 2021

eolivelli left a comment

eolivelli Sep 14, 2021

eolivelli Sep 14, 2021

eolivelli Sep 14, 2021

eolivelli Sep 14, 2021

eolivelli Sep 14, 2021

eolivelli left a comment

[Transaction] Transaction coordinator fence mechanism. #11357

[Transaction] Transaction coordinator fence mechanism. #11357

Conversation

congbobo184 commented Jul 19, 2021 • edited Loading

Verifying this change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anonymitaet commented Jul 20, 2021

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

congbobo184 commented Jul 19, 2021 •

edited

Loading