[fix][broker] fix No such ledger exception #16420

lordcheng10 · 2022-07-06T13:10:38Z

Motivation

In the log we found No such ledger exception:

The reasons are as follows:
1. Frequent Full GC occurs on the broker, causing zk to time out：
22:29:06.119 [main-EventThread] ERROR org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase - ZooKeeper client connection to the ZooKeeper server has expired!
2.When the create leadger is completed, the ledger will be put into ledgers and update currentLedger，the updateLedgersListAfterRollover method is executed, and the metadata to zookeeper fails to write：

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

Lines 1484 to 1489 in 4c958a9

    
           log.info("[{}] Created new ledger {}", name, lh.getId()); 
        
           ledgers.put(lh.getId(), LedgerInfo.newBuilder().setLedgerId(lh.getId()).setTimestamp(0).build()); 
        
           currentLedger = lh; 
        
           currentLedgerEntries = 0; 
        
           currentLedgerSize = 0;

3.In the failure callback method operationFailed, the corresponding ledger will be removed from the ledgers, but the currentLedger still points to the ledger that failed to create, but the ledger will be deleted here through bookKeeper.asyncDeleteLedger. When reading data, it will be read through the currentLedger. Since the ledger has been deleted, the final error is reported: No such ledger

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

Lines 1509 to 1522 in 4c958a9

    
           public void operationFailed(MetaStoreException e) { 
        
               log.warn("[{}] Error updating meta data with the new list of ledgers: {}", name, e.getMessage()); 
        
               // Remove the ledger, since we failed to update the list 
        
               ledgers.remove(lh.getId()); 
        
               mbean.startDataLedgerDeleteOp(); 
        
               bookKeeper.asyncDeleteLedger(lh.getId(), (rc1, ctx1) -> { 
        
                   mbean.endDataLedgerDeleteOp(); 
        
                   if (rc1 != BKException.Code.OK) { 
        
                       log.warn("[{}] Failed to delete ledger {}: {}", name, lh.getId(), 
        
                               BKException.getMessage(rc1)); 
        
                   } 
        
               }, null); 
        
               if (e instanceof BadVersionException) {

Modifications

After the create ledger is created and the metadata is written to the zookeeper successfully, the currentLedger is updated.

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

github-actions · 2022-07-06T13:11:30Z

@lordcheng10 Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

lordcheng10 · 2022-07-06T13:19:23Z

@hangc0276 PTAL，thanks!

lordcheng10 · 2022-07-06T13:19:40Z

@codelipenghui @eolivelli PTAL,thanks!

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

hangc0276

Nice catch!

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

lordcheng10 · 2022-07-06T15:29:43Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-06T16:59:28Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-06T17:00:12Z

/pulsarbot run-failure-checks

merlimat · 2022-07-06T21:00:54Z

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

                        log.debug("[{}] Updating of ledgers list after create complete. version={}", name, stat);
                    }
                    ledgersStat = stat;
+                    ledgers.put(lh.getId(), LedgerInfo.newBuilder().setLedgerId(lh.getId()).setTimestamp(0).build());


@lordcheng10 Adding to the ledgers map cannot be done here, because that map is used when writing to the z-node (eg: buildManagedLedgerInfo(Map<Long, LedgerInfo> ledgers).

fixed. PTAL,thanks! @merlimat

I still have concerns because in this method, if the ledger is not persisted to the metadata, it means the ledger cannot be used. But we put it in the ledger map; it is not persistent and may be used by other operations under multi-threaded conditions.

However, I'm not sure what it will affect. Just a minor concern. Please feel free to go on.

Your concern is right, I will try to update the zookeeper metadata with a temporary ledgersTmp, and update the ledgers after the zookeeper is successfully written. @mattisonchao @merlimat

Agree with @mattisonchao. This may lead to data loss.

Putting the newly created ledger into the NavigableMap<Long, LedgerInfo> ledgers, the new ledger can be seen immediately which means can write data into this ledger, and then if the meta-store operation failed, the ledger will be removed from both the ledgers and bookie, all data in this ledger are lost.

fixed. @merlimat @hangc0276 @aloyszhang @mattisonchao PTAL,thanks!

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

mattisonchao · 2022-07-07T08:33:46Z

@hangc0276

This pull request made a lot of change after your review. Could you have time to review it again?

lordcheng10 · 2022-07-07T08:48:05Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-07T09:38:27Z

I think we are not handling this condition well:
If the process runs at this line, we continue to retry later. But at the same time, the ledgers is changed. However, we still use old data to update. I'm not sure if it is a big problem. But I think we have to clarify it.
I think I have to give a change request. @lordcheng10 Could you help to confirm it?

This is a problem, I made a modification, please review it again.
The metadata information written to zookeeper should be constructed after the metadataMutex is obtained. @mattisonchao

mattisonchao

LGTM +1

lordcheng10 · 2022-07-07T12:00:32Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-07T12:57:03Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-07T16:39:39Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-08T01:50:14Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-07-08T03:14:56Z

/pulsarbot run-failure-checks

* fix No such ledger exception * move currentLedgerEntries and currentLedgerSize * move ledgers.put to operationComplete * fix ledgers.put * use ledgersTmp to write zookkeeper,after sucess,update ldgers * update updateLedgersListAfterRollover * put lh to ledgersTmp * extend buildManagedLedgerInfo * getManagedLedgerInfo(newLedger) after get metadataMutex (cherry picked from commit 2e5fbbc)

* fix No such ledger exception * move currentLedgerEntries and currentLedgerSize * move ledgers.put to operationComplete * fix ledgers.put * use ledgersTmp to write zookkeeper,after sucess,update ldgers * update updateLedgersListAfterRollover * put lh to ledgersTmp * extend buildManagedLedgerInfo * getManagedLedgerInfo(newLedger) after get metadataMutex

* fix No such ledger exception * move currentLedgerEntries and currentLedgerSize * move ledgers.put to operationComplete * fix ledgers.put * use ledgersTmp to write zookkeeper,after sucess,update ldgers * update updateLedgersListAfterRollover * put lh to ledgersTmp * extend buildManagedLedgerInfo * getManagedLedgerInfo(newLedger) after get metadataMutex (cherry picked from commit 2e5fbbc) (cherry picked from commit 41c0cf3)

* fix No such ledger exception * move currentLedgerEntries and currentLedgerSize * move ledgers.put to operationComplete * fix ledgers.put * use ledgersTmp to write zookkeeper,after sucess,update ldgers * update updateLedgersListAfterRollover * put lh to ledgersTmp * extend buildManagedLedgerInfo * getManagedLedgerInfo(newLedger) after get metadataMutex

* fix No such ledger exception * move currentLedgerEntries and currentLedgerSize * move ledgers.put to operationComplete * fix ledgers.put * use ledgersTmp to write zookkeeper,after sucess,update ldgers * update updateLedgersListAfterRollover * put lh to ledgersTmp * extend buildManagedLedgerInfo * getManagedLedgerInfo(newLedger) after get metadataMutex (cherry picked from commit 2e5fbbc)

fix No such ledger exception

ded6d8f

github-actions bot added doc-label-missing doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jul 6, 2022

hangc0276 reviewed Jul 6, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Show resolved Hide resolved

move currentLedgerEntries and currentLedgerSize

140f29c

hangc0276 approved these changes Jul 6, 2022

View reviewed changes

hangc0276 requested review from 315157973, Jason918, codelipenghui, dlg99, eolivelli, merlimat and zymap July 6, 2022 13:51

hangc0276 assigned lordcheng10 Jul 6, 2022

hangc0276 added area/broker release/2.8.4 release/2.10.2 release/2.9.4 labels Jul 6, 2022

hangc0276 added this to the 2.11.0 milestone Jul 6, 2022

mattisonchao approved these changes Jul 6, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Outdated Show resolved Hide resolved

move ledgers.put to operationComplete

c698df7

merlimat reviewed Jul 6, 2022

View reviewed changes

fix ledgers.put

dc505b7

Technoboy- approved these changes Jul 7, 2022

View reviewed changes

Jason918 reviewed Jul 7, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Outdated Show resolved Hide resolved

mattisonchao requested changes Jul 7, 2022

View reviewed changes

extend buildManagedLedgerInfo

b36da7b

lordcheng10 requested review from Jason918 and mattisonchao July 7, 2022 08:01

Jason918 approved these changes Jul 7, 2022

View reviewed changes

mattisonchao approved these changes Jul 7, 2022

View reviewed changes

mattisonchao requested a review from hangc0276 July 7, 2022 08:34

getManagedLedgerInfo(newLedger) after get metadataMutex

1200640

mattisonchao approved these changes Jul 7, 2022

View reviewed changes

Jason918 merged commit 2e5fbbc into apache:master Jul 8, 2022

codelipenghui added the cherry-picked/branch-2.10 label Jul 10, 2022

mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jul 15, 2022

BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 29, 2022

poorbarcode mentioned this pull request Jan 29, 2023

[fix] [ml] The atomicity of multiple fields of ml is broken #19346

Merged

4 tasks

	log.info("[{}] Created new ledger {}", name, lh.getId());
	ledgers.put(lh.getId(), LedgerInfo.newBuilder().setLedgerId(lh.getId()).setTimestamp(0).build());
	currentLedger = lh;
	currentLedgerEntries = 0;
	currentLedgerSize = 0;

	public void operationFailed(MetaStoreException e) {
	log.warn("[{}] Error updating meta data with the new list of ledgers: {}", name, e.getMessage());
	// Remove the ledger, since we failed to update the list
	ledgers.remove(lh.getId());
	mbean.startDataLedgerDeleteOp();
	bookKeeper.asyncDeleteLedger(lh.getId(), (rc1, ctx1) -> {
	mbean.endDataLedgerDeleteOp();
	if (rc1 != BKException.Code.OK) {
	log.warn("[{}] Failed to delete ledger {}: {}", name, lh.getId(),
	BKException.getMessage(rc1));
	}
	}, null);

	if (e instanceof BadVersionException) {

[fix][broker] fix No such ledger exception #16420

[fix][broker] fix No such ledger exception #16420

Uh oh!

Conversation

lordcheng10 commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Uh oh!

github-actions bot commented Jul 6, 2022

Uh oh!

lordcheng10 commented Jul 6, 2022

Uh oh!

lordcheng10 commented Jul 6, 2022

Uh oh!

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lordcheng10 commented Jul 6, 2022

Uh oh!

lordcheng10 commented Jul 6, 2022

Uh oh!

lordcheng10 commented Jul 6, 2022

Uh oh!

merlimat Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

mattisonchao Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aloyszhang Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattisonchao commented Jul 7, 2022

Uh oh!

lordcheng10 commented Jul 7, 2022

Uh oh!

lordcheng10 commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattisonchao left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Jul 7, 2022

Uh oh!

lordcheng10 commented Jul 7, 2022

Uh oh!

lordcheng10 commented Jul 7, 2022

Uh oh!

lordcheng10 commented Jul 8, 2022

Uh oh!

lordcheng10 commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lordcheng10 commented Jul 6, 2022 •

edited

Loading

lordcheng10 Jul 7, 2022 •

edited

Loading

lordcheng10 commented Jul 7, 2022 •

edited

Loading