Add getTxnID method in Transaction.java #11438

liangyepianzhou · 2021-07-23T07:30:50Z

Motivation

In order to facilitate the use of transaction, it is more convenient to obtain TxnId

implement

Add getTxnID（） method in Transaction.java and implement in TransactionImp.

Test

Write a test in transactionTest .

codelipenghui · 2021-07-23T07:40:28Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

+
+    @Test
+    public void testGetTxnID() throws Exception {
+        Awaitility.await().atMost(4, TimeUnit.SECONDS).until(()->{


Why do we need Awaitility here? I think the transaction can be created directly.

TransactionCoodinator may not be ready

Ok, I see. Could you please help add a comment? This will help others to understand.
Related to this fix: #11357

codelipenghui · 2021-07-23T07:41:47Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

+            }
+            return true;
+        });
+        Transaction transaction = pulsarClient.newTransaction()


Why need to create the transaction again?

codelipenghui · 2021-07-23T07:43:03Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

+        Assert.assertEquals(txnID.getLeastSigBits(), 1);
+        Assert.assertEquals(txnID.getMostSigBits(), 0);


We will always get transaction ID (1,0) here? I mean we can just check the txnID is not null.

In fact it should always be this

eolivelli

very good.

I have a comment about the test.
can you please take a look ?

eolivelli · 2021-07-23T08:01:41Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

+    public void testGetTxnID() throws Exception {
+        Awaitility.await().atMost(4, TimeUnit.SECONDS).until(()->{
+            try {
+                Transaction transaction = pulsarClient.newTransaction()


@congbobo184 @codelipenghui
I believe that this is not a good expected behaviour for a client.

it should be the newTransaction() method that waits for one Coordinator to be available up to a given (configurable) timeout.
otherwise the API is difficult to understand and to use.

can we add an utility method in TransactionTestBase like "waitForCoordinatorToBeAvailable" ?

Very good suggestion.
but this PR is mainly for getTxnID, and waitForCoordinatorToBeAvailable will be optimized later

I understand your concern @liangyuanpeng
But if we leave here this Awaiatilty loop we won't remove it in the future probably.

It is better to fix it now

If you prefer we can create a separate patch to be committed before this patch.
But adding that method should be easy

Different tests may require different numbers of broker and tc.
Is it more appropriate to mention a pr to fix this problem ?
If you don't mind, I will do this later.

I have optimized waitForCoordinatorToBeAvailable method for TransactionTest.

congbobo184

LGTM! @eolivelli waitForCoordinatorToBeAvailable can be implemented in another pr. what do you think? @liangyuanpeng Are you interested in implementing it in the next PR??

codelipenghui · 2021-07-23T09:58:06Z

I think we can implement the waitForCoordinatorToBeAvailable first and then merge this PR.

eolivelli

very better !

I left some final comments.
We are on our way !
thank you

eolivelli · 2021-07-23T10:54:56Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

@@ -53,23 +61,26 @@

    private static final String TENANT = "tnx";
    private static final String NAMESPACE1 = TENANT + "/ns1";
+    private static final int NUM_BROKER = 1;
+    private static final int NUM_TC_PER_ = 1;


is there a typo here ? why this variable ends with a '_' ?

terribly sorry.
I overlooked this detail.
Thanks for pointing out.

eolivelli · 2021-07-23T10:55:10Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

        this.internalSetup();

-        String[] brokerServiceUrlArr = getPulsarServiceList().get(0).getBrokerServiceUrl().split(":");
+        String[] brokerServiceUrlArr = getPulsarServiceList().get(NUM_BROKER - 1).getBrokerServiceUrl().split(":");


why we are picking the last and not the first ?

Excuse me.
I can't think of the difference between them, I just use NUM_BROKER-1 uniformly.
But maybe, if there is no difference, it is a great philosophy to modify as little as possible.

eolivelli

very good
I left my final feedback.

then the patch is good to go from my point of you

thanks for your patience

eolivelli · 2021-07-23T12:15:44Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

@@ -53,23 +61,26 @@

    private static final String TENANT = "tnx";
    private static final String NAMESPACE1 = TENANT + "/ns1";
+    private static final int NUM_BROKER = 1;
+    private static final int NUM_TC_PER = 1;


what about NUM_PARTITIONS ?

Thank you for your help and guidance,
your meticulous spirit and outstanding design concept have benefited me a lot

eolivelli · 2021-07-23T12:15:53Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

@@ -53,23 +61,26 @@

    private static final String TENANT = "tnx";
    private static final String NAMESPACE1 = TENANT + "/ns1";
+    private static final int NUM_BROKER = 1;


what about NUM_BROKERS ?

codelipenghui · 2021-07-23T12:57:08Z

@eolivelli Thanks for the review. Could you please help take a look again?

eolivelli

LGTM

thank you

(cherry picked from commit 1977a84)

Add getTxnID method in Transaction.java

f6ecfd7

congbobo184 requested review from eolivelli, congbobo184, codelipenghui and gaoran10 July 23, 2021 07:31

codelipenghui requested changes Jul 23, 2021

View reviewed changes

codelipenghui assigned liangyepianzhou Jul 23, 2021

codelipenghui added this to the 2.9.0 milestone Jul 23, 2021

codelipenghui added the release/2.8.1 label Jul 23, 2021

Modify the Awaitility.await() in TransactionTest and doc in Transaction

f5c5499

eolivelli requested changes Jul 23, 2021

View reviewed changes

congbobo184 approved these changes Jul 23, 2021

View reviewed changes

liangyepianzhou requested review from eolivelli and codelipenghui July 23, 2021 08:24

Optimize waitForCoordinatorToBeAvailable method in TransactionTest

8fd8e65

eolivelli requested changes Jul 23, 2021

View reviewed changes

Fixed the details in TransactionTest

1b377cd

eolivelli reviewed Jul 23, 2021

View reviewed changes

codelipenghui approved these changes Jul 23, 2021

View reviewed changes

Fixed the problem of naming in TransactionTest

ee4daff

codelipenghui requested a review from eolivelli July 23, 2021 12:55

codelipenghui added the area/transaction label Jul 23, 2021

eolivelli approved these changes Jul 27, 2021

View reviewed changes

eolivelli merged commit 1977a84 into apache:master Jul 27, 2021

codelipenghui pushed a commit that referenced this pull request Jul 30, 2021

Add getTxnID method in Transaction.java (#11438)

38d3389

(cherry picked from commit 1977a84)

codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 30, 2021

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

Add getTxnID method in Transaction.java (apache#11438)

f83c0aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getTxnID method in Transaction.java #11438

Add getTxnID method in Transaction.java #11438

liangyepianzhou commented Jul 23, 2021 •

edited

codelipenghui Jul 23, 2021

liangyepianzhou Jul 23, 2021

codelipenghui Jul 23, 2021

codelipenghui Jul 23, 2021

codelipenghui Jul 23, 2021

liangyepianzhou Jul 23, 2021

eolivelli left a comment

eolivelli Jul 23, 2021

liangyepianzhou Jul 23, 2021

eolivelli Jul 23, 2021

eolivelli Jul 23, 2021

liangyepianzhou Jul 23, 2021 •

edited

liangyepianzhou Jul 23, 2021

congbobo184 left a comment •

edited

codelipenghui commented Jul 23, 2021

eolivelli left a comment

eolivelli Jul 23, 2021

liangyepianzhou Jul 23, 2021

eolivelli Jul 23, 2021

liangyepianzhou Jul 23, 2021

eolivelli left a comment

eolivelli Jul 23, 2021

liangyepianzhou Jul 23, 2021

eolivelli Jul 23, 2021

codelipenghui commented Jul 23, 2021

eolivelli left a comment

		Assert.assertEquals(txnID.getLeastSigBits(), 1);
		Assert.assertEquals(txnID.getMostSigBits(), 0);

Add getTxnID method in Transaction.java #11438

Add getTxnID method in Transaction.java #11438

Conversation

liangyepianzhou commented Jul 23, 2021 • edited

Motivation

implement

Test

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

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

liangyepianzhou Jul 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

congbobo184 left a comment • edited

Choose a reason for hiding this comment

codelipenghui commented Jul 23, 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

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

codelipenghui commented Jul 23, 2021

eolivelli left a comment

Choose a reason for hiding this comment

liangyepianzhou commented Jul 23, 2021 •

edited

liangyepianzhou Jul 23, 2021 •

edited

congbobo184 left a comment •

edited