Add sanity check in GC to prevent active ledger deletion #15
Conversation
| @@ -350,6 +348,23 @@ public void readLedgerMetadata(final long ledgerId, final GenericCallback<Ledger | |||
| readLedgerMetadata(ledgerId, readCb, null); | |||
| } | |||
|
|
|||
| @Override | |||
| public void existLedgerMetadata(final long ledgerId, final GenericCallback<Boolean> callback) { | |||
| LOG.warn("Fail to check {} exists in zk {}", ledgerId, BKException.getMessage(rc)); | ||
| } | ||
| latch.countDown(); | ||
| semaphore.release(); |
There was a problem hiding this comment.
I don't think we need a semaphore at all. Except during unit tests the gc method is called by scheduleAtFixedRate, which guarantees that later calls to gc will not happen until the previous ones finish.
if garbageCleaner.clean was inside the callback then the semaphore would make since, but with the CountDownLatch this is a blocking call, so it really doesn't.
There was a problem hiding this comment.
yes, i missed to remove it after adding latch. will fix it.
|
I think this is similar as the change Salesforce made in apache/bookkeeper@1f8b26d fyi |
|
@sijie That change still does not address cases where due to some other bug a valid ledger is deleted. |
| private void gcLedgerSafely(GarbageCleaner garbageCleaner, long ledgerId, LedgerManager ledgerManager) throws InterruptedException { | ||
| CountDownLatch latch = new CountDownLatch(1); | ||
| AtomicBoolean ledgerDeleted = new AtomicBoolean(false); | ||
| ledgerManager.existsLedgerMetadata(ledgerId, (rc, exists) -> { |
Actually, I think it's the same behavior, because it forces to read the ledger metadata, which means reading the znode |
|
I see. Did we merge that pull request? I see it got closed, so not sure if it got approved eventually. |
|
apache#876 was approved and merged. the gitsha I pointed out is the change that already exists in latest apache/master. I think the change in apache and the change here are almost same, either merge this one to yahoo or cherry-pick the apache change works for me. |
|
thanks for pointing out the commit. I think checking just |
|
@rdhabalia could you take a look at #22 and see if it is good enough to replace this? It is a backport of the change that went into the apache repo that you suggested we look at instead. |
Motivation
Sometimes, due to unexpected bugs in GC, GC deletes active ledgers (ledger metadata still exist in ZK) from the bookie and it results into the data-loss.
We have seen this scenario multiple times:
(1) Lexicographical sorting at ledgerManager
(2) Recently we have seen that LongHierarchicalLedgerManager doesn't handle ledger directory traversing if we manually cleans up empty directory which doesn't contain any children.
eg:
We implemented a cron job to delete ledger directory from zookeeper, when it does not contain any children in zookeeper. e.g:
/ledgers/000/0000/0042/0003In the above sequence, after processing 0001 and 0002 nodes, when LongHierarchicalLedgerManager iterator sees the next node(0003) does not exist, it assumes no other nodes starting from 0003 till 9999 exists under /ledgers/000/0000/0042. This triggers bookie to delete any ledgers from 0004 onwards present in that bookie.
So, we have seen data loss multiple times which can be prevented if GC can perform a sanity check before cleaning ledger from the bookie.
Modifications
Add sanity check before cleaning up the ledger.
Result
It can prevent possible data loss due to any bug in GC.