Skip to content
This repository has been archived by the owner on Apr 30, 2019. It is now read-only.

YBK-183: Garbage collector deletes active ledgers present in zookeeper #17

Open
wants to merge 1 commit into
base: yahoo-4.3
Choose a base branch
from

Conversation

jai1
Copy link

@jai1 jai1 commented Jan 19, 2018

When all level 3 nodes are deleted from a given level 2 node, the hasNext returns null which compels the bookkeeper to incorrectly GC the ledgers

Based on the position of the level 2 node either of the two can happen:-
a. If the level2 node is the first level2 node scanned (smallest number) then the bookkeeper GCs all nodes herewith - Fixed in this PR

b. if level2 node is in the middle (not the first) the bookkeeper throws an exception - and GC activity stops altogether and those ledgers are never deleted - Needs to be analyzed and fixed

I am able to reproduce these scenarios manually - will see if I can add unit tests

@revans2

@merlimat
Copy link
Collaborator

@jai1 Change LGTM. Would be possible to test this by manually removing the intermediate z-nodes?

Also, please submit this on apache/bookkeeper as well so that it gets included into master.

@merlimat
Copy link
Collaborator

@jai1 @rdhabalia Is this getting targeted for 1.22 release?

@revans2
Copy link
Collaborator

revans2 commented Feb 6, 2018

@jai1 This change does look good. Are you going to address b? Also this code is out in the Apache repo as well and needs to be addressed there too. Do you have plans to put up a pull request there as well?

@jai1
Copy link
Author

jai1 commented Feb 8, 2018

@revans2 - I don't have permissions to merge PRs on bookkeeper

I have raised a PR for fixing issue 2 (#23) but didn't get time to test it

Also this code is out in the Apache repo

Can you point me to the exact branch cause - I checked master and the code there doesn't use preload.

rdhabalia pushed a commit to rdhabalia/bookkeeper that referenced this pull request Sep 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants