Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][client] Make seek wait for reconnect #18202

Closed
wants to merge 1 commit into from

Conversation

nodece
Copy link
Member

@nodece nodece commented Oct 26, 2022

Motivation

When doing seek operation, the client needs to seed a seek command to the broker. When one seeks is successful, the broker disconnects the client, and then the client reconnects and resubscribes to the broker based on the seek position.

Right now, the consumer responds to the caller after sending a seek command succeeds, this should be incorrect behavior.
The right behavior should be to wait for reconnect and resubscribe, then respond to the caller.

Modifications

Based on the global CompletableFuture and seekMessageId check the seek operation to wait for reconnect and resubscribe.

When reconnecting is done, make the CompletableFuture to complete, when resubscribing is done, cleanup the seekMessageId.

Documentation

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

Matching PR in forked repository

PR in forked repository: nodece#9

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 26, 2022
@nodece nodece force-pushed the make-seek-wait-reconnect branch from fddbb68 to 68ba899 Compare October 26, 2022 09:23
@Technoboy- Technoboy- added this to the 2.12.0 milestone Nov 2, 2022
@Technoboy- Technoboy- closed this Nov 2, 2022
@Technoboy- Technoboy- reopened this Nov 2, 2022
@Technoboy- Technoboy- added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/client labels Nov 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #18202 (68ba899) into master (6c65ca0) will increase coverage by 5.44%.
The diff coverage is 35.90%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18202      +/-   ##
============================================
+ Coverage     34.91%   40.35%   +5.44%     
- Complexity     5707     8685    +2978     
============================================
  Files           607      687      +80     
  Lines         53396    67396   +14000     
  Branches       5712     7219    +1507     
============================================
+ Hits          18644    27200    +8556     
- Misses        32119    37161    +5042     
- Partials       2633     3035     +402     
Flag Coverage Δ
unittests 40.35% <35.90%> (+5.44%) ⬆️

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

Impacted Files Coverage Δ
...org/apache/bookkeeper/mledger/LedgerOffloader.java 0.00% <ø> (ø)
...che/bookkeeper/mledger/LedgerOffloaderFactory.java 0.00% <ø> (ø)
...pache/bookkeeper/mledger/LedgerOffloaderStats.java 0.00% <ø> (ø)
...ookkeeper/mledger/LedgerOffloaderStatsDisable.java 0.00% <ø> (ø)
...a/org/apache/bookkeeper/mledger/ManagedCursor.java 85.71% <ø> (ø)
...che/bookkeeper/mledger/ManagedLedgerException.java 41.17% <ø> (ø)
...bookkeeper/mledger/ManagedLedgerFactoryConfig.java 86.66% <ø> (ø)
...g/apache/bookkeeper/mledger/ManagedLedgerInfo.java 22.22% <ø> (ø)
...he/bookkeeper/mledger/OffloadedLedgerMetadata.java 0.00% <ø> (ø)
...ava/org/apache/bookkeeper/mledger/ScanOutcome.java 0.00% <ø> (ø)
... and 741 more

@lhotari
Copy link
Member

lhotari commented Nov 23, 2022

@nodece Do you think that this PR addresses #10671 ?

@nodece
Copy link
Member Author

nodece commented Nov 23, 2022

@nodece Do you think that this PR addresses #10671 ?

I think this PR can fix race condition of seeking.

@lhotari
Copy link
Member

lhotari commented Nov 23, 2022

is there any relationship to #16757?

@nodece
Copy link
Member Author

nodece commented Nov 24, 2022

is there any relationship to #16757?

Right, but this PR fixes some issues on the client side.

Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

I think we should reconsider seek. This PR only solves some problems, but it does not solve all seek and reconnect concurrency problems.

@nodece
Copy link
Member Author

nodece commented Dec 26, 2022

I think we should reconsider seek. This PR only solves some problems, but it does not solve all seek and reconnect concurrency problems.

Do you have any suggestions?

@congbobo184
Copy link
Contributor

I have no idea about that. [PIP-194] https://lists.apache.org/thread/97o9t4ltkds5pfq41l9xbbd31t41qm8w discuss how to solve this problem.

@nodece
Copy link
Member Author

nodece commented Dec 28, 2022

I have no idea about that. [PIP-194] https://lists.apache.org/thread/97o9t4ltkds5pfq41l9xbbd31t41qm8w discuss how to solve this problem.

I think this is a different issue, I just fix the concurrency problems of seeking on the client side.

@github-actions
Copy link

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

@nodece
Copy link
Member Author

nodece commented Mar 25, 2023

No reviewer, so close this PR.

@nodece nodece closed this Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client doc-not-needed Your PR changes do not impact docs ready-to-test Stale type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants