Skip to content

Conversation

@mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Mar 14, 2022

Motivation

The original ledger creation design was lazy, meaning that the ledger was created when a new write operation was requested.

} else if (state == State.ClosedLedger) {
// No ledger and no pending operations. Create a new ledger
if (STATE_UPDATER.compareAndSet(this, State.ClosedLedger, State.CreatingLedger)) {
log.info("[{}] Creating a new ledger", name);
this.lastLedgerCreationInitiationTimestamp = System.currentTimeMillis();
mbean.startDataLedgerCreateOp();
asyncCreateLedger(bookKeeper, config, digestType, this, Collections.emptyMap());
}
} else {

However, the current rollCurrentLedgerIfFull logic is to create a ledger regardless of the condition (even if ManagedLedgerImpl#pendingAddEntries is empty)

@VisibleForTesting
@Override
public void rollCurrentLedgerIfFull() {
log.info("[{}] Start checking if current ledger is full", name);
if (currentLedgerEntries > 0 && currentLedgerIsFull()
&& STATE_UPDATER.compareAndSet(this, State.LedgerOpened, State.ClosingLedger)) {
currentLedger.asyncClose(new AsyncCallback.CloseCallback() {
@Override
public void closeComplete(int rc, LedgerHandle lh, Object o) {
checkArgument(currentLedger.getId() == lh.getId(), "ledgerId %s doesn't match with "
+ "acked ledgerId %s", currentLedger.getId(), lh.getId());
if (rc == BKException.Code.OK) {
log.debug("Successfully closed ledger {}", lh.getId());
} else {
log.warn("Error when closing ledger {}. Status={}", lh.getId(), BKException.getMessage(rc));
}
ledgerClosed(lh);
createLedgerAfterClosed();
}
}, System.nanoTime());
}
}

Therefore, we need to change to lazy creation according to the original design.

Modifications

  • Remove createLedgerAfterClosed invoke in the rollCurrentLedgerIfFull method.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • no-need-doc

@mattisonchao mattisonchao changed the title Fix rollCurrentLedgerIfFull to follow lazy creation of ledger Change rollCurrentLedgerIfFull logic to follow lazy creation of ledger Mar 14, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 14, 2022
@mattisonchao mattisonchao reopened this Mar 14, 2022
@mattisonchao
Copy link
Member Author

mattisonchao commented Mar 14, 2022

@eolivelli
Since this PR changes the behaviour of rollCurrentLedgerIfFull, the test ManagedCursorTest#testFindNewestMatchingAfterLedgerRollover needs some changes and I need your review.

@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 14, 2022
@mattisonchao
Copy link
Member Author

@codelipenghui @BewareMyPower @eolivelli @lhotari @michaeljmarshall @Technoboy-

This change affects some tests, PTAL :)

mattisonchao and others added 3 commits March 15, 2022 19:05
…l/ManagedLedgerTest.java

Co-authored-by: Yunze Xu <xyzinfernity@163.com>
@BewareMyPower BewareMyPower merged commit 30f7f00 into apache:master Mar 16, 2022
aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Mar 21, 2022
…edger (apache#14672)

### Motivation

The original ledger creation design was lazy, meaning that the ledger was created when a new write operation was requested.

https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L778-L786

However, the current ``rollCurrentLedgerIfFull`` logic is to create a ledger regardless of the condition (even if ``ManagedLedgerImpl#pendingAddEntries`` is empty)

https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1669-L1692

Therefore, we need to change to lazy creation according to the original design.

### Modifications

- Remove ``createLedgerAfterClosed`` invoke in the ``rollCurrentLedgerIfFull`` method.
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…edger (apache#14672)

### Motivation

The original ledger creation design was lazy, meaning that the ledger was created when a new write operation was requested.

https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L778-L786

However, the current ``rollCurrentLedgerIfFull`` logic is to create a ledger regardless of the condition (even if ``ManagedLedgerImpl#pendingAddEntries`` is empty)

https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1669-L1692

Therefore, we need to change to lazy creation according to the original design.

### Modifications

- Remove ``createLedgerAfterClosed`` invoke in the ``rollCurrentLedgerIfFull`` method.
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request May 17, 2022
nicoloboschi pushed a commit that referenced this pull request Jun 8, 2022
…edger (#14672)

### Motivation

The original ledger creation design was lazy, meaning that the ledger was created when a new write operation was requested.

https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L778-L786

However, the current ``rollCurrentLedgerIfFull`` logic is to create a ledger regardless of the condition (even if ``ManagedLedgerImpl#pendingAddEntries`` is empty)

https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1669-L1692

Therefore, we need to change to lazy creation according to the original design.

### Modifications

- Remove ``createLedgerAfterClosed`` invoke in the ``rollCurrentLedgerIfFull`` method.

(cherry picked from commit 30f7f00)
nicoloboschi added a commit that referenced this pull request Jun 8, 2022
Technoboy- pushed a commit that referenced this pull request Aug 4, 2022
Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants