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.

Sign up for GitHub

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

Jump to bottom

[fix][broker] Fix getPositionAfterN infinite loop. #17971

Merged
merged 5 commits into from
Oct 13, 2022
Merged

[fix][broker] Fix getPositionAfterN infinite loop. #17971

merged 5 commits into from
Oct 13, 2022

Conversation

Technoboy-
Copy link
Contributor

Fixes #17967

Master Issue: #17967

Motivation

The root cause is here(line-3365) :

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

Lines 3364 to 3366 in 08f5766

Long lid = ledgers.ceilingKey(currentLedgerId + 1);
currentLedgerId = lid != null ? lid : (ledgers.lastKey() + 1);
currentEntryId = 0;

The currentLedgerId should be :

currentLedgerId = lid != null ? lid : (ledgers.lastKey());

Documentation

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

Matching PR in forked repository

PR in forked repository: https://github.com/Technoboy-/pulsar/pull/9

@Technoboy- Technoboy- self-assigned this Oct 9, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Oct 9, 2022
codelipenghui
codelipenghui reviewed Oct 10, 2022
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Great catch!

Is it able to add an unit test?

@Technoboy-
Copy link
Contributor Author

Great catch!

Is it able to add an unit test?

We already have the test testGetPositionAfterN

@codelipenghui
Copy link
Contributor

codelipenghui commented Oct 10, 2022 •
edited
Loading

We already have the test testGetPositionAfterN

@Technoboy- So why did the test get passed before? I think the test can't cover the case that introduces the infinite loop?

@Technoboy-
Copy link
Contributor Author

We already have the test testGetPositionAfterN

@Technoboy- So why did the test get passed before? I think the test can't cover the case that introduces the infinite loop?

Ah, yes, right. Sorry for this. I have added the test.

@Technoboy- Technoboy- changed the title [fix][ML] Fix getPositionAfterN infinite loop. [fix][broker] Fix getPositionAfterN infinite loop. Oct 11, 2022
@codelipenghui
Copy link
Contributor

@Technoboy- Please check the failed test

@Technoboy- Technoboy- force-pushed the refactor-getPositionAfterN branch from 03ef4e9 to 9a44a73 Compare October 12, 2022 06:59
@Technoboy-
Copy link
Contributor Author

@Technoboy- Please check the failed test

Yes, I have fixed by #17993

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2022 •
edited
Loading

Codecov Report

Merging #17971 (bac6889) into master (6c65ca0) will increase coverage by 10.87%.
The diff coverage is 60.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #17971       +/-   ##
=============================================
+ Coverage     34.91%   45.78%   +10.87%     
- Complexity     5707    17561    +11854     
=============================================
  Files           607     1573      +966     
  Lines         53396   128213    +74817     
  Branches       5712    14100     +8388     
=============================================
+ Hits          18644    58708    +40064     
- Misses        32119    63406    +31287     
- Partials       2633     6099     +3466
Flag Coverage Δ
unittests 45.78% <60.00%> (+10.87%) ⬆️

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

Impacted Files Coverage Δ
.../service/SystemTopicBasedTopicPoliciesService.java 62.65% <0.00%> (+11.06%) ⬆️
...g/apache/pulsar/compaction/CompactedTopicImpl.java 69.28% <0.00%> (+58.57%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerBase.java 21.95% <0.00%> (ø)
...broker/delayed/InMemoryDelayedDeliveryTracker.java 65.00% <75.00%> (+65.00%) ⬆️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 71.29% <80.00%> (ø)
...rg/apache/pulsar/broker/service/BrokerService.java 57.27% <100.00%> (+9.27%) ⬆️
...ar/broker/loadbalance/impl/BundleSplitterTask.java 60.00% <0.00%> (-17.78%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 44.71% <0.00%> (-11.06%) ⬇️
...e/pulsar/broker/service/EntryBatchIndexesAcks.java 82.14% <0.00%> (-10.72%) ⬇️
...ava/org/apache/pulsar/io/redis/sink/RedisSink.java 49.18% <0.00%> (-6.56%) ⬇️
... and 1113 more

Jason918
Jason918 approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@Technoboy- Technoboy- force-pushed the refactor-getPositionAfterN branch from 84f4711 to bac6889 Compare October 13, 2022 05:18
@Technoboy- Technoboy- merged commit c732852 into apache:master Oct 13, 2022
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Oct 13, 2022
Technoboy- added a commit that referenced this pull request Oct 13, 2022
@Technoboy-
[fix][broker] Fix getPositionAfterN infinite loop. (#17971)
1adf042
congbobo184 pushed a commit that referenced this pull request Nov 8, 2022
@Technoboy-
[fix][broker] Fix getPositionAfterN infinite loop. (#17971)
825df41 
(cherry picked from commit c732852)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 8, 2022
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
@Technoboy-
[fix][broker] Fix getPositionAfterN infinite loop. (#17971)
c0ff7de 
(cherry picked from commit c732852)
liangyepianzhou pushed a commit that referenced this pull request Dec 7, 2022
@Technoboy- @liangyepianzhou
[fix][broker] Fix getPositionAfterN infinite loop. (#17971)
49a3a13 
(cherry picked from commit c732852)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
@Technoboy- @nicoloboschi
[fix][broker] Fix getPositionAfterN infinite loop. (apache#17971)
2e38ef2 
(cherry picked from commit c732852)
(cherry picked from commit 49a3a13)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
@Technoboy- @nicoloboschi
[fix][broker] Fix getPositionAfterN infinite loop. (apache#17971)
62c64ee 
(cherry picked from commit c732852)
(cherry picked from commit 49a3a13)
@poorbarcode poorbarcode mentioned this pull request May 8, 2023
Closed
@Technoboy- Technoboy- deleted the refactor-getPositionAfterN branch November 11, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderzc coderzc coderzc approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@Jason918 Jason918 Jason918 approved these changes

Assignees

@Technoboy- Technoboy-

Labels
cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.8.5 release/2.9.4 release/2.10.3
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.

[BUG]Unknown reason causes getPositionAfterN infinite loop
8 participants
@Technoboy- @codelipenghui @codecov-commenter @Jason918 @poorbarcode @coderzc @congbobo184 @liangyepianzhou