[fix][client] Make the whole grabCnx() progress atomic #20595

BewareMyPower · 2023-06-16T15:30:23Z

Motivation

In ConnectionHandler, there is a Connection interface whose methods will be called after the connection in grabCnx is established, the implementation of Connection might send some requests after the connection is established. For example, the consumer will send the CommandSubscribe request in connectionOpened. However, the whole process is not atomic, which leads to the message lost reported in #20591.

Modifications

Modify the Connection#connectionOpened interface to return a CompletableFuture.

The returned future should be completed once the implementation has done everything, e.g. for the consumer, the future should only be completed after receiving the response for CommandSubscribe.

In grabCnx, the ConnectionHandler could only connect to the broker once the whole process is completed.

Add ConnectionHandlerTest to verify the behavior.

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: BewareMyPower#29

BewareMyPower · 2023-06-16T15:42:16Z

With this patch, we don't need any synchronization when implementing handleNewConnection (the combination of connectionOpened and connectionFailed) in future.

BewareMyPower · 2023-06-16T16:46:00Z

There are some failed tests, I will fix them ASAP.

codelipenghui · 2023-06-19T14:16:32Z

/pulsarbot run-failure-checks

BewareMyPower · 2023-06-26T06:33:04Z

PTAL @codelipenghui @poorbarcode

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConnectionHandler.java

tisonkun

LGTM

BewareMyPower · 2023-06-29T03:24:49Z

I made new changes to avoid race condition described in #20595 (comment)

The root cause is that for a grabCnx operation, duringConnect.set(false) could be called twice and reconnectLater could be called by the connection implementation. So I removed the reconnectLater calls in the Connection#connectionOpened implementations and just complete the future exceptionally so that reconnectLater will be called later.

            cnxFuture.thenCompose(cnx -> connection.connectionOpened(cnx))
                    .thenAccept(__ -> duringConnect.set(false))
                    .exceptionally(this::handleConnectionError);

All possible cases are:

cnxFuture completes with null
1.1 connectionOpened completes with null, thenAccept will set duringConnect with false.
1.2 connectionOpened completes exceptionally, exceptionally will call reconnectLater, which sets duringConnect with false.
cnxFuture completes exceptionally, exceptionally will call reconnectLater, which sets duringConnect with false.

Ideally, reconnectLater should be private, but it seems hard to remove the reconnectLater calls in TransactionMetaStoreHandler.

PTAL again. @poorbarcode @lifepuzzlefun @tisonkun @Technoboy-

### Motivation In `ConnectionHandler`, there is a `Connection` interface whose methods will be called after the connection in `grabCnx` is established, the implementation of `Connection` might send some requests after the connection is established. For example, the consumer will send the `CommandSubscribe` request in `connectionOpened`. However, the whole process is not atomic, which leads to the message lost reported in apache#20591. ### Modifications Modify the `Connection` interface to have a single method: ```java CompletableFuture<Void> handleNewConnection(ClientCnx cnx, PulsarClientException e); ``` The returned future should be completed once the implementation has done everything, e.g. for the consumer, the future should only be completed after receiving the response for `CommandSubscribe`. In `grabCnx`, the `ConnectionHandler` could only connect to the broker once the whole process is completed. Add `ConnectionHandlerTest` to verify the behavior.

tisonkun

I see your point. LGTM.

When there are some reviewers you may try to do accumulative commits instead of force push; thus it's easy to review the changeset compared to the last review..

You can "merge master" instead of rebase and the final "squash and merge" would squash everything into one commit for you.

BewareMyPower · 2023-06-29T16:29:02Z

When there are some reviewers you may try to do accumulative commits instead of force push; thus it's easy to review the changeset compared to the last review..

I know it so I didn't push it by force before. But since there were many new commits from the approved commit before, I think it might be better to review for a completely new commit.

The PR changed a lot

BewareMyPower · 2023-06-30T03:05:35Z

The failed test testMsgRateExpired will be fixed by #20629

codecov-commenter · 2023-06-30T08:38:16Z

Codecov Report

Merging #20595 (48540d0) into master (0e60340) will increase coverage by 0.53%.
The diff coverage is 77.85%.

@@             Coverage Diff              @@
##             master   #20595      +/-   ##
============================================
+ Coverage     72.60%   73.13%   +0.53%     
- Complexity    32018    32076      +58     
============================================
  Files          1855     1869      +14     
  Lines        138569   138883     +314     
  Branches      15250    15273      +23     
============================================
+ Hits         100605   101578     +973     
+ Misses        29945    29262     -683     
- Partials       8019     8043      +24

Flag	Coverage Δ
inttests	`24.11% <41.42%> (+<0.01%)`	⬆️
systests	`25.07% <25.71%> (?)`
unittests	`72.41% <77.50%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java	`76.17% <0.00%> (-0.08%)`	⬇️
...ulsar/client/impl/TransactionMetaStoreHandler.java	`68.44% <44.44%> (-0.03%)`	⬇️
...rg/apache/pulsar/client/impl/TopicListWatcher.java	`65.21% <46.15%> (+0.82%)`	⬆️
.../service/SystemTopicBasedTopicPoliciesService.java	`76.48% <50.00%> (-1.19%)`	⬇️
...ar/common/naming/TopicBundleAssignmentFactory.java	`66.66% <66.66%> (ø)`
...n/java/org/apache/pulsar/admin/cli/CliCommand.java	`57.95% <71.42%> (-1.60%)`	⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java	`90.47% <76.19%> (+3.24%)`	⬆️
...va/org/apache/pulsar/client/impl/ProducerImpl.java	`82.76% <80.00%> (+0.59%)`	⬆️
.../metadata/bookkeeper/PulsarRegistrationClient.java	`80.85% <81.11%> (-4.11%)`	⬇️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java	`77.25% <84.61%> (+0.75%)`	⬆️
... and 10 more

... and 155 files with indirect coverage changes

tisonkun · 2023-06-30T08:54:07Z

@poorbarcode Thanks for your update.

Merging...

Then you can rebase your PR onto this one :D

liangyepianzhou · 2023-07-07T14:04:14Z

@BewareMyPower, the TopicListWatcher.java is not included in the branch 2.10. So this PR does not need to cherry-pick to branch 2.10, right?

liangyepianzhou · 2023-07-08T03:42:06Z

@BewareMyPower, the TopicListWatcher.java is not included in the branch 2.10. So this PR does not need to cherry-pick to branch 2.10, right?

Oh, my fault. We can cherry-pick this PR to branch 2.10. TopicListWatcher.java is not part of the core modification.

(cherry picked from commit 2bede01)

(cherry picked from commit 2bede01) (cherry picked from commit d5631c9)

BewareMyPower added type/bug The PR fixed a bug or issue reported a bug area/client release/2.10.5 release/2.11.2 release/3.0.2 labels Jun 16, 2023

BewareMyPower added this to the 3.1.0 milestone Jun 16, 2023

BewareMyPower requested review from hangc0276, eolivelli, codelipenghui, tisonkun, Demogorgon314, poorbarcode and mattisonchao June 16, 2023 15:30

BewareMyPower self-assigned this Jun 16, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 16, 2023

BewareMyPower mentioned this pull request Jun 16, 2023

[fix] [client] Messages lost when consumer reconnect #20591

Closed

4 tasks

BewareMyPower marked this pull request as draft June 16, 2023 16:26

BewareMyPower marked this pull request as ready for review June 18, 2023 13:39

BewareMyPower marked this pull request as draft June 19, 2023 07:24

BewareMyPower marked this pull request as ready for review June 19, 2023 08:33

codelipenghui added the ready-to-test label Jun 19, 2023

tisonkun reviewed Jun 26, 2023

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConnectionHandler.java Outdated Show resolved Hide resolved

tisonkun reviewed Jun 26, 2023

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConnectionHandler.java Outdated Show resolved Hide resolved

Technoboy- reviewed Jun 27, 2023

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConnectionHandler.java Outdated Show resolved Hide resolved

tisonkun approved these changes Jun 27, 2023

View reviewed changes

Technoboy- previously approved these changes Jun 27, 2023

View reviewed changes

BewareMyPower requested a review from tisonkun June 29, 2023 03:14

BewareMyPower force-pushed the bewaremypower/grab-cnx-atomic branch from dc9c062 to 6116870 Compare June 29, 2023 15:31

tisonkun approved these changes Jun 29, 2023

View reviewed changes

Merge branch 'master' into bewaremypower/grab-cnx-atomic

48540d0

poorbarcode approved these changes Jun 30, 2023

View reviewed changes

tisonkun merged commit 2bede01 into apache:master Jun 30, 2023

poorbarcode mentioned this pull request Jun 30, 2023

[fix] [client] Messages lost when consumer reconnect #20695

Merged

4 tasks

Technoboy- pushed a commit that referenced this pull request Jul 3, 2023

[fix][client] Make the whole grabCnx() progress atomic (#20595)

c595a51

Technoboy- added the cherry-picked/branch-3.0 label Jul 3, 2023

Technoboy- pushed a commit that referenced this pull request Jul 4, 2023

[fix][client] Make the whole grabCnx() progress atomic (#20595)

daa3066

Technoboy- added the cherry-picked/branch-2.11 label Jul 4, 2023

Technoboy- added a commit to Technoboy-/pulsar that referenced this pull request Jul 5, 2023

Fix cherry-pick apache#20595 causes compile issue

f9faef8

liangyepianzhou removed the release/2.10.5 label Jul 7, 2023

liangyepianzhou pushed a commit that referenced this pull request Jul 8, 2023

[fix][client] Make the whole grabCnx() progress atomic (#20595)

d5631c9

(cherry picked from commit 2bede01)

liangyepianzhou added cherry-picked/branch-2.10 release/2.10.5 labels Jul 8, 2023

RobertIndie added release/3.0.1 and removed release/3.0.2 labels Jul 13, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 17, 2023

[fix][client] Make the whole grabCnx() progress atomic (apache#20595)

4d735f4

(cherry picked from commit 2bede01) (cherry picked from commit d5631c9)

BewareMyPower mentioned this pull request Oct 14, 2023

Delay the timing of setting reconnectionPending to false to avoid double attempt at reconnecting apache/pulsar-client-cpp#328

Merged

BewareMyPower deleted the bewaremypower/grab-cnx-atomic branch December 13, 2023 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][client] Make the whole grabCnx() progress atomic #20595

[fix][client] Make the whole grabCnx() progress atomic #20595

BewareMyPower commented Jun 16, 2023 •

edited

Loading

BewareMyPower commented Jun 16, 2023

BewareMyPower commented Jun 16, 2023

codelipenghui commented Jun 19, 2023

BewareMyPower commented Jun 26, 2023

tisonkun left a comment

BewareMyPower commented Jun 29, 2023

tisonkun left a comment •

edited

Loading

BewareMyPower commented Jun 29, 2023 •

edited

Loading

BewareMyPower commented Jun 30, 2023

codecov-commenter commented Jun 30, 2023

tisonkun commented Jun 30, 2023

liangyepianzhou commented Jul 7, 2023 •

edited

Loading

liangyepianzhou commented Jul 8, 2023

[fix][client] Make the whole grabCnx() progress atomic #20595

[fix][client] Make the whole grabCnx() progress atomic #20595

Conversation

BewareMyPower commented Jun 16, 2023 • edited Loading

Motivation

Modifications

Documentation

Matching PR in forked repository

BewareMyPower commented Jun 16, 2023

BewareMyPower commented Jun 16, 2023

codelipenghui commented Jun 19, 2023

BewareMyPower commented Jun 26, 2023

tisonkun left a comment

Choose a reason for hiding this comment

BewareMyPower commented Jun 29, 2023

tisonkun left a comment • edited Loading

Choose a reason for hiding this comment

BewareMyPower commented Jun 29, 2023 • edited Loading

BewareMyPower commented Jun 30, 2023

codecov-commenter commented Jun 30, 2023

Codecov Report

tisonkun commented Jun 30, 2023

liangyepianzhou commented Jul 7, 2023 • edited Loading

liangyepianzhou commented Jul 8, 2023

BewareMyPower commented Jun 16, 2023 •

edited

Loading

tisonkun left a comment •

edited

Loading

BewareMyPower commented Jun 29, 2023 •

edited

Loading

liangyepianzhou commented Jul 7, 2023 •

edited

Loading