Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete #12113

lordcheng10 · 2021-09-21T07:08:20Z

Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete

eolivelli

LGTM

Anonymitaet · 2021-09-22T03:32:41Z

@lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

lordcheng10 · 2021-09-22T06:15:52Z

@lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

OK, I'd be happy to update the document

lordcheng10 · 2021-09-22T12:37:32Z

@lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@Anonymitaet I searched for the document about this method in the project, but did not find any document related to this method, so there is no need to modify the document.

michaeljmarshall

It almost seems to me that this method should have been used to close the cursor ledger here:

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

Lines 2235 to 2250 in 0a1ae98

    
           public void operationComplete() { 
        
               log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name, 
        
                       markDeletePosition, cursorLedger.getId()); 
        
               cursorLedger.asyncClose((rc, lh, ctx1) -> { 
        
                   callback.closeComplete(ctx); 
        
                   if (rc == BKException.Code.OK) { 
        
                       log.info("[{}][{}] Closed cursor-ledger {}", ledger.getName(), name, 
        
                               cursorLedger.getId()); 
        
                   } else { 
        
                       log.warn("[{}][{}] Failed to close cursor-ledger {}: {}", ledger.getName(), name, 
        
                               cursorLedger.getId(), BKException.getMessage(rc)); 
        
                   } 
        
               }, ctx); 
        
           }

.

The logic of this asyncCloseCursorLedger method is quite similar, but not identical, to the above code block. Specifically, this asyncCloseCursorLedger method has metrics and fails the callback if the close fails. I wonder if its worth updating the persistPositionWhenClosing method to use this currently unused method.

If we do decide to remove asyncCloseCursorLedger, I think if we should either update persistPositionWhenClosing to increment/decrement the cursorLedgerCloseOp or remove the cursorLedgerCloseOp metric, as well as its references, from the ledger.mbean, since it is only updated within this method.

@eolivelli - do you have any thoughts on the right direction here? Thanks.

lordcheng10 · 2021-09-23T05:14:24Z

It almost seems to me that this method should have been used to close the cursor ledger here:

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

Lines 2235 to 2250 in 0a1ae98

public void operationComplete() {

log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,

markDeletePosition, cursorLedger.getId());

cursorLedger.asyncClose((rc, lh, ctx1) -> {

callback.closeComplete(ctx);

if (rc == BKException.Code.OK) {

log.info("[{}][{}] Closed cursor-ledger {}", ledger.getName(), name,

cursorLedger.getId());

} else {

log.warn("[{}][{}] Failed to close cursor-ledger {}: {}", ledger.getName(), name,

cursorLedger.getId(), BKException.getMessage(rc));

}

}, ctx);

}

.
The logic of this asyncCloseCursorLedger method is quite similar, but not identical, to the above code block. Specifically, this asyncCloseCursorLedger method has metrics and fails the callback if the close fails. I wonder if its worth updating the persistPositionWhenClosing method to use this currently unused method.

If we do decide to remove asyncCloseCursorLedger, I think if we should either update persistPositionWhenClosing to increment/decrement the cursorLedgerCloseOp or remove the cursorLedgerCloseOp metric, as well as its references, from the ledger.mbean, since it is only updated within this method.

@eolivelli - do you have any thoughts on the right direction here? Thanks.

@michaeljmarshall
It seems that it is better to use asyncCloseCursorLedger to replace the asynchronous close in operationComplete. The modified operationComplete should look like this:

                    public void operationComplete() {
                        log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,
                                markDeletePosition, cursorLedger.getId());
                        asyncCloseCursorLedger(callback, ctx);
                    }

michaeljmarshall · 2021-09-23T18:30:22Z

I agree that it seems appropriate to use asyncCloseCursorLedger to replace the code in the operationComplete method. My only question is if failing the callback is the right behavior. It seems correct to me, but I'd like to verify with someone more familiar with this part of the code base.

…te method

lordcheng10 · 2021-09-27T03:01:18Z

It almost seems to me that this method should have been used to close the cursor ledger here:

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

Lines 2235 to 2250 in 0a1ae98

public void operationComplete() {

log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,

markDeletePosition, cursorLedger.getId());

cursorLedger.asyncClose((rc, lh, ctx1) -> {

callback.closeComplete(ctx);

if (rc == BKException.Code.OK) {

log.info("[{}][{}] Closed cursor-ledger {}", ledger.getName(), name,

cursorLedger.getId());

} else {

log.warn("[{}][{}] Failed to close cursor-ledger {}: {}", ledger.getName(), name,

cursorLedger.getId(), BKException.getMessage(rc));

}

}, ctx);

}

.
The logic of this asyncCloseCursorLedger method is quite similar, but not identical, to the above code block. Specifically, this asyncCloseCursorLedger method has metrics and fails the callback if the close fails. I wonder if its worth updating the persistPositionWhenClosing method to use this currently unused method.
If we do decide to remove asyncCloseCursorLedger, I think if we should either update persistPositionWhenClosing to increment/decrement the cursorLedgerCloseOp or remove the cursorLedgerCloseOp metric, as well as its references, from the ledger.mbean, since it is only updated within this method.
@eolivelli - do you have any thoughts on the right direction here? Thanks.

@michaeljmarshall
It seems that it is better to use asyncCloseCursorLedger to replace the asynchronous close in operationComplete. The modified operationComplete should look like this:
                    public void operationComplete() {
                        log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,
                                markDeletePosition, cursorLedger.getId());
                        asyncCloseCursorLedger(callback, ctx);
                    }

@codelipenghui @lhotari @merlimat @BewareMyPower - PTAL, thanks.

michaeljmarshall

LGTM. From my perspective, this PR changes the callback behavior to the expected behavior. We should call the closeFailed callback method when we fail to close the cursor ledger.

@lordcheng10 - now that we're changing direction on this PR, we should update the title so that the commit message will align with the actual change. It might also be a good idea to update the asyncCloseCursorLedger method to log the two removed log lines.

Based on quickly looking at usage for the persistPositionWhenClosing method, it looks like the only code that will change fundamental behavior is here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java

Lines 120 to 135 in 0460472

    
           cursor.asyncClose(new AsyncCallbacks.CloseCallback() { 
        
               @Override 
        
               public void closeComplete(Object ctx) { 
        
                   try { 
        
                       managedLedger.close(); 
        
                   } catch (Exception e) { 
        
                       completableFuture.completeExceptionally(e); 
        
                   } 
        
                   completableFuture.complete(null); 
        
               } 
        
               @Override 
        
               public void closeFailed(ManagedLedgerException exception, Object ctx) { 
        
                   completableFuture.completeExceptionally(exception); 
        
               } 
        
           }, null);

No other methods have branching behavior in their callback based on closeComplete vs closeFailure.

@congbobo184 - you wrote the above code block for transactions. Will it be a problem to change behavior as this PR proposes?

…ines.

lordcheng10 · 2021-09-28T01:56:31Z

LGTM. From my perspective, this PR changes the callback behavior to the expected behavior. We should call the closeFailed callback method when we fail to close the cursor ledger.

@lordcheng10 - now that we're changing direction on this PR, we should update the title so that the commit message will align with the actual change. It might also be a good idea to update the asyncCloseCursorLedger method to log the two removed log lines.

Based on quickly looking at usage for the persistPositionWhenClosing method, it looks like the only code that will change fundamental behavior is here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java

Lines 120 to 135 in 0460472

cursor.asyncClose(new AsyncCallbacks.CloseCallback() {

@Override

public void closeComplete(Object ctx) {

try {

managedLedger.close();

} catch (Exception e) {

completableFuture.completeExceptionally(e);

}

completableFuture.complete(null);

}

@Override

public void closeFailed(ManagedLedgerException exception, Object ctx) {

completableFuture.completeExceptionally(exception);

}

}, null);

No other methods have branching behavior in their callback based on closeComplete vs closeFailure.

@congbobo184 - you wrote the above code block for transactions. Will it be a problem to change behavior as this PR proposes?

As you described, I added two logs in asyncCloseCursorLedger and updated the title

lordcheng10 · 2021-09-28T07:08:21Z

@eolivelli, @codelipenghui, @BewareMyPower, @sijie, @hangc0276, @merlimat - PTAL, thanks.

lordcheng10 · 2021-09-28T16:47:54Z

/pulsarbot run-failure-checks

lordcheng10 · 2021-09-29T01:32:50Z

CI has passed @BewareMyPower @michaeljmarshall PTAL ，thanks！

BewareMyPower · 2021-09-29T01:46:15Z

@congbobo184 could you answer the question from @michaeljmarshall ?

lordcheng10 · 2021-10-12T11:40:21Z

@congbobo184 PTAL ,thanks!

congbobo184 · 2021-10-14T08:36:11Z

LGTM！#12113 (comment) this seem to a bug. This is a bug, can you fix it easily? Future completed twice.thanks

lordcheng10 · 2021-10-14T08:53:26Z

LGTM！#12113 (comment) this seem to a bug. This is a bug, can you fix it easily? Future completed twice.thanks

Yeah. I will fix

lordcheng10 · 2021-10-14T09:09:14Z

LGTM！#12113 (comment) this seem to a bug. This is a bug, can you fix it easily? Future completed twice.thanks

@congbobo184 fix bug: #12362

* up/master: (37 commits) re-enabling integration tests for Sinks (apache#12307) [PIP 95][Issue 12040][web] Topic lookup with listener header (apache#12072) Fix the master CI broken with update dispatch rate block issue (apache#12360) Fix message being ignored when the non-persistent topic reader reconnect. (apache#12348) Fix log format. (apache#12346) [website][upgrade]feat: docs migration - version-2.7.2 Concepts and Architecture (apache#12354) [website][upgrade] feat: full docs migration for version 2.8.0 (apache#12359) [website][upgrade]feat: dynamic replace version info before build (apache#12337) Fix flaky tests: ElasticSearchClientTests (apache#12347) Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete (apache#12113) fix-npe-ZkBookieRackAffinityMapping (apache#11947) [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128) [website][upgrade]feat: docs migration - Development (apache#12320) Update delete inactive topic configuration documentation (apache#12350) [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056) Added Debezium Source for MS SQL Server (apache#12256) Fix: flaky oracle tests (apache#12306) [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341) [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335) feat(cli): add restart command to pulsar-daemon (apache#12279) ... # Conflicts: # site2/website-next/docusaurus.config.js # site2/website-next/versioned_sidebars/version-2.7.2-sidebars.json # site2/website-next/versions.json

…in the ManagedCursorImpl.VoidCallback#operationComplete (apache#12113) The org.apache.bookkeeper.mledger.impl.ManagedCursorImpl#asyncCloseCursorLedger method is an unused method, delete the redundant method

Remove unused method: asyncCloseCursorLedger

9a02b89

eolivelli approved these changes Sep 21, 2021

View reviewed changes

eolivelli requested review from codelipenghui and lhotari September 21, 2021 09:39

michaeljmarshall requested changes Sep 22, 2021

View reviewed changes

use asyncCloseCursorLedger to replace the code in the operationComple…

0eccccd

…te method

lordcheng10 requested a review from eolivelli September 24, 2021 07:14

michaeljmarshall approved these changes Sep 27, 2021

View reviewed changes

lordcheng10 changed the title ~~Remove unused method: asyncCloseCursorLedger~~ Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete Sep 28, 2021

update the asyncCloseCursorLedger method to log the two removed log l…

90ddc78

…ines.

BewareMyPower assigned lordcheng10 Sep 28, 2021

BewareMyPower added the area/broker label Sep 28, 2021

BewareMyPower approved these changes Sep 28, 2021

View reviewed changes

BewareMyPower added the doc-not-needed Your PR changes do not impact docs label Sep 28, 2021

BewareMyPower added this to the 2.9.0 milestone Sep 28, 2021

eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021

congbobo184 approved these changes Oct 14, 2021

View reviewed changes

congbobo184 merged commit 508ef82 into apache:master Oct 14, 2021

lordcheng10 mentioned this pull request Oct 14, 2021

Future completed twice in the method of impl.MLPendingAckStore#closeAsync #12362

Merged

	public void operationComplete() {
	log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,
	markDeletePosition, cursorLedger.getId());
	cursorLedger.asyncClose((rc, lh, ctx1) -> {
	callback.closeComplete(ctx);

	if (rc == BKException.Code.OK) {
	log.info("[{}][{}] Closed cursor-ledger {}", ledger.getName(), name,
	cursorLedger.getId());
	} else {
	log.warn("[{}][{}] Failed to close cursor-ledger {}: {}", ledger.getName(), name,
	cursorLedger.getId(), BKException.getMessage(rc));
	}
	}, ctx);

	}

	cursor.asyncClose(new AsyncCallbacks.CloseCallback() {
	@Override
	public void closeComplete(Object ctx) {
	try {
	managedLedger.close();
	} catch (Exception e) {
	completableFuture.completeExceptionally(e);
	}
	completableFuture.complete(null);
	}

	@Override
	public void closeFailed(ManagedLedgerException exception, Object ctx) {
	completableFuture.completeExceptionally(exception);
	}
	}, null);

Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete #12113

Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete #12113

Uh oh!

Conversation

lordcheng10 commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Anonymitaet commented Sep 22, 2021

Uh oh!

lordcheng10 commented Sep 22, 2021

Uh oh!

lordcheng10 commented Sep 22, 2021

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaeljmarshall commented Sep 23, 2021

Uh oh!

lordcheng10 commented Sep 27, 2021

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lordcheng10 commented Sep 28, 2021

Uh oh!

lordcheng10 commented Sep 28, 2021

Uh oh!

lordcheng10 commented Sep 29, 2021

Uh oh!

BewareMyPower commented Sep 29, 2021

Uh oh!

lordcheng10 commented Oct 12, 2021

Uh oh!

congbobo184 commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lordcheng10 commented Oct 14, 2021

Uh oh!

lordcheng10 commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lordcheng10 commented Sep 21, 2021 •

edited

Loading

lordcheng10 commented Sep 23, 2021 •

edited

Loading

lordcheng10 commented Sep 28, 2021 •

edited

Loading

congbobo184 commented Oct 14, 2021 •

edited

Loading

lordcheng10 commented Oct 14, 2021 •

edited

Loading