make ledger rollover check task internal#8946
Merged
sijie merged 6 commits intoJan 7, 2021
Merged
Conversation
hangc0276
requested review from
aahmed-se,
codelipenghui,
merlimat and
sijie
December 14, 2020 01:22
Contributor
Author
|
@wuzhanpeng Please help take a look, thanks. |
wuzhanpeng
reviewed
Dec 14, 2020
| }), timeoutSec, timeoutSec, TimeUnit.SECONDS); | ||
| } | ||
|
|
||
| if (config.getMaximumRolloverTimeMs() > 0) { |
Contributor
There was a problem hiding this comment.
I think it would be better to separate the timing task of checking that current ledger is full from scheduleTimeoutTask and put it in the ledger initialize method.
Contributor
Author
There was a problem hiding this comment.
Ok, I move it into individual method.
Contributor
Author
|
/pulsarbot run-failure-checks |
3 similar comments
Contributor
Author
|
/pulsarbot run-failure-checks |
Contributor
Author
|
/pulsarbot run-failure-checks |
Contributor
Author
|
/pulsarbot run-failure-checks |
sijie
requested changes
Dec 23, 2020
| /** | ||
| * Roll current ledger if it is full | ||
| */ | ||
| void rollCurrentLedgerIfFull(); |
Member
There was a problem hiding this comment.
@hangc0276 we shouldn't remove a method from an interface directly. we should keep the method and mark it as deprecated.
Contributor
Author
There was a problem hiding this comment.
Ok, i move it back.
Contributor
Author
|
/pulsarbot run-failure-checks |
1 similar comment
Contributor
Author
|
/pulsarbot run-failure-checks |
sijie
approved these changes
Jan 4, 2021
hangc0276
force-pushed
the
make_ledger_rollover_check_task_internal
branch
from
January 7, 2021 05:20
9fe915a to
534326d
Compare
Contributor
Author
|
/pulsarbot run-failure-checks |
1 similar comment
Contributor
Author
|
/pulsarbot run-failure-checks |
Contributor
Author
|
/pulsarbot run-failure-checks |
merlimat
pushed a commit
to merlimat/pulsar
that referenced
this pull request
Apr 6, 2021
Fix apache#7195 ### Changes 1. add a schedulerTask to rollover the ledger in `ManagedLedgerImpl` instead of `BrokerService` 2. add the test.
eolivelli
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 20, 2021
Fix apache#7195 1. add a schedulerTask to rollover the ledger in `ManagedLedgerImpl` instead of `BrokerService` 2. add the test. (cherry picked from commit b4ef76e)
codelipenghui
pushed a commit
that referenced
this pull request
Jul 9, 2021
The PR #11116 couldn't be cherry-picked to `branch-2.7`, because there are too many conflicts. In PR #8946 the ledger rollover task had been moved from `BrokerService` to `ManagedLedgerImpl`, and this PR is based on it. ### Motivation Currently, the ledger rollover scheduled task will execute before reach the ledger maximum rollover time, this will cause the ledger doesn't roll over in time. ### Modifications Only make the ledger rollover scheduled task after the ledger created successfully. If the scheduled task was executed when there is no entry in the current ledger, the scheduled task will not be re-executed, and if there is new entry is added the ledger will rollover.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #7195
Changes
ManagedLedgerImplinstead ofBrokerService