Skip to content

[fix][txn] Allow producer enable send timeout in transaction - #16519

Merged
codelipenghui merged 2 commits into
apache:masterfrom
congbobo184:congbobo184_open_send_txn_message_timeout
Jul 11, 2022
Merged

codelipenghui merged 2 commits into
apache:masterfrom
congbobo184:congbobo184_open_send_txn_message_timeout

Conversation

@congbobo184

@congbobo184 congbobo184 commented Jul 11, 2022 •

Copy link
Copy Markdown
Contributor

Motivation

In the actual scenario, because the transaction will timeout, the transaction cannot block sending messages all the time. So we send messages with txn need to throw TimeoutException

Modifications

send messages with txn don't need set producer config .sendTimeout(0, TimeUnit.SECONDS)

Verifying this change

add send messages with txn throw TimeoutException test

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

  • Does this pull request introduces a new feature? (yes)

  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

  • If a feature is not applicable for documentation, explain why?

  • If a feature is not documented yet in this PR, please create a follow-up issue for adding the documentation

  • doc-not-needed

@congbobo184 congbobo184 added area/transaction doc-not-needed Your PR changes do not impact docs labels Jul 11, 2022
@congbobo184 congbobo184 added this to the 2.11.0 milestone Jul 11, 2022
@congbobo184 congbobo184 self-assigned this Jul 11, 2022

@gaoran10 gaoran10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions Bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Jul 11, 2022
@github-actions

Copy link
Copy Markdown

@congbobo184 Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@congbobo184 congbobo184 added the doc-not-needed Your PR changes do not impact docs label Jul 11, 2022
@codelipenghui codelipenghui changed the title [change][txn] Send message with txn can throw TimeoutException [fix][txn] Allow producer enable send timeout in transaction Jul 11, 2022
@codelipenghui codelipenghui added the type/bug The PR fixed a bug or issue reported a bug label Jul 11, 2022
@codelipenghui
codelipenghui merged commit bbf2a47 into apache:master Jul 11, 2022
codelipenghui pushed a commit that referenced this pull request Jul 11, 2022
codelipenghui pushed a commit that referenced this pull request Jul 11, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 11, 2022
@nicoloboschi

Copy link
Copy Markdown
Contributor

@codelipenghui There's a test failing on branch-2.10 related to this pull

java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertTrue(Assert.java:45)
	at org.testng.Assert.assertTrue(Assert.java:55)
	at org.apache.pulsar.client.impl.TransactionEndToEndTest.testSendTxnMessageTimeout(TransactionEndToEndTest.java:1099)

Could you verify ?

@nicoloboschi

Copy link
Copy Markdown
Contributor

I found the reason, it's related to the test itself. Will send a fix soon

congbobo184 pushed a commit that referenced this pull request Jul 14, 2022
…MessageTimeout (only release branches) (#16570)

### Motivation

`TransactionEndToEndTest#testSendTxnMessageTimeout` fails on releases branch after #16519 has been cherry-picked. 

```
java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertTrue(Assert.java:45)
	at org.testng.Assert.assertTrue(Assert.java:55)
	at org.apache.pulsar.client.impl.TransactionEndToEndTest.testSendTxnMessageTimeout(TransactionEndToEndTest.java:1099)
```

The reason is that the mock setup must be slightly different since the `ProducerImpl` is not exactly the same between master and branch-2.10.
wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022
mattisonchao pushed a commit that referenced this pull request Jul 15, 2022
…MessageTimeout (only release branches) (#16570)

### Motivation

`TransactionEndToEndTest#testSendTxnMessageTimeout` fails on releases branch after #16519 has been cherry-picked. 

```
java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertTrue(Assert.java:45)
	at org.testng.Assert.assertTrue(Assert.java:55)
	at org.apache.pulsar.client.impl.TransactionEndToEndTest.testSendTxnMessageTimeout(TransactionEndToEndTest.java:1099)
```

The reason is that the mock setup must be slightly different since the `ProducerImpl` is not exactly the same between master and branch-2.10.
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jul 15, 2022
…MessageTimeout (only release branches) (apache#16570)

### Motivation

`TransactionEndToEndTest#testSendTxnMessageTimeout` fails on releases branch after apache#16519 has been cherry-picked.

```
java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertTrue(Assert.java:45)
	at org.testng.Assert.assertTrue(Assert.java:55)
	at org.apache.pulsar.client.impl.TransactionEndToEndTest.testSendTxnMessageTimeout(TransactionEndToEndTest.java:1099)
```

The reason is that the mock setup must be slightly different since the `ProducerImpl` is not exactly the same between master and branch-2.10.

(cherry picked from commit c83bede)
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jul 18, 2022
…MessageTimeout (only release branches) (apache#16570)

### Motivation

`TransactionEndToEndTest#testSendTxnMessageTimeout` fails on releases branch after apache#16519 has been cherry-picked.

```
java.lang.AssertionError: expected [true] but found [false]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertTrue(Assert.java:45)
	at org.testng.Assert.assertTrue(Assert.java:55)
	at org.apache.pulsar.client.impl.TransactionEndToEndTest.testSendTxnMessageTimeout(TransactionEndToEndTest.java:1099)
```

The reason is that the mock setup must be slightly different since the `ProducerImpl` is not exactly the same between master and branch-2.10.

(cherry picked from commit c83bede)
lhotari added a commit to lhotari/pulsar that referenced this pull request Sep 7, 2026
…ands

Follow-up to the review on apache#26480, pulsar-perf half.

- `transaction-v4` had no functional coverage: every existing reference was
  metadata-only, so the transaction binding itself was unpinned. Adds
  `PerformanceTransactionV4Test` with a commit run and an `-abort` run against
  plain `persistent://` topics. The commit run asserts both halves are durable
  (produced messages visible, consume backlog down by one per transaction); the
  abort run asserts neither is (nothing visible, backlog unchanged, and all ten
  messages redeliverable on the tool's own subscription, which is what separates
  a real abort from a transaction that was merely never ended). Mutation checked:
  making `sendMessage` or `acknowledgeAsync` ignore the transaction, dropping the
  acknowledgement entirely, or making `abortTransaction` a no-op each fail it.

- `PerformanceConsumerBase` had dropped the `consumerType` attribute from the
  per-topic "Adding consumers" line, which the pre-split `PerformanceConsumer`
  logged. Restores it through a `consumerTypeForLog()` hook: the base reports
  the shared `--subscription-type`, `PerformanceConsumer` overrides it with
  `scalableConsumerType`, so the V5 line matches the pre-split output again.

- `PerformanceTransactionBase` interrupted `Thread.currentThread()` from its
  failure callbacks. That is the worker thread for a V5 ack, whose future is
  already complete when returned, but a client-internal thread for a v4 ack and
  for the send and end-transaction futures on both clients. All three callbacks
  now capture the worker thread, which is the thread `runWorker` invokes them on.

- `transaction-v4`'s and `produce-v4`'s `sendTimeout(0)` was annotated "a send
  timeout and a transaction are mutually exclusive on the v4 client". That guard
  was removed in apache#16519; the setting is still right because a send timeout fails
  the send on its own schedule and takes the transaction with it, so the comments
  now say that instead.

- `PerformanceV4CommandsTest`: the backlog assertion now waits with Awaitility
  (the exit latch is released before the client close that flushes the acks, and
  the 30s join is unchecked), and the delivery-time assertions also pin each
  message's payload so a future reordering fails legibly.

- The `LATENCY_HISTOGRAM_SIGNIFICANT_DIGITS` rationale still described the
  static recorders that apache#26466 replaced with instance fields in the same commit
  that added the comment. Rewords both copies, with the figure re-measured on
  HdrHistogram 2.2.2: 16.00 MB (1h in micros) and 14.00 MB (10d in millis) at
  5 digits, not the pre-apache#26466 ranges' 11-22 MB.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/transaction cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.10.2 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants