Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BOOKKEEPER-552: 64 Bits Ledger ID Generation #112

Closed
wants to merge 1 commit into from

Conversation

@knusbaum
Copy link
Contributor

knusbaum commented Feb 17, 2017

This PR adds 63-bit ID generation to Bookkeeper and modifies the hierarchical ledger structure to be backwards compatible.

@reddycharan

This comment has been minimized.

Copy link
Contributor

reddycharan commented Feb 17, 2017

Hey @knusbaum we already pushed our commit for 63 bit ledgerID. Can you please check that -

42e8f12

@knusbaum

This comment has been minimized.

Copy link
Contributor Author

knusbaum commented Feb 17, 2017

@reddycharan Yes, this builds on that. It makes it backwards compatible and builds the 64-bit id generation stuff which was missing.

@reddycharan

This comment has been minimized.

Copy link
Contributor

reddycharan commented Feb 17, 2017

@knusbaum can you please explain briefly, what you are doing in this change and why LongHierarchicalLedgermanager is being removed.

@knusbaum

This comment has been minimized.

Copy link
Contributor Author

knusbaum commented Feb 18, 2017

LongHierarchicalLedgerManager is being moved to HierarchicalLedgerManager and HierarchicalLedgerManager is being moved to LegacyHierarchicalLedgerManager.

LongHierarchicalLedgerManager (now HierarchicalLedgerManager) has been made backwards-compatible so that existing Bookkeeper instances running HierarchicalLedgerManager can be upgraded to the new code, which will allow 64-bit IDs.

This is achieved by having the HierarchicalLedgerManager proxy requests to the LegacyHierarchicalLedgerManager for ids which are less than Integer.MAX_VALUE
For ids greater than that, the new code is used.

We also add a new id generator. The existing ZkLedgerIdGenerator is limited to 31 bits (about 2 million) ledgers because ZooKeeper's sequential id generator uses a signed int internally. IDs start going negative if you overflow it, which messes some stuff up.

The new ID generator uses a multi-level directory scheme to overcome this.

Copy link

revans2 left a comment

Only one minor comment.

@@ -142,6 +144,7 @@ public static BKException create(int code) {
int UnauthorizedAccessException = -102;
int UnclosedFragmentException = -103;
int WriteOnReadOnlyBookieException = -104;
int LedgerIdOverflowException = -106;

This comment has been minimized.

Copy link
@revans2

revans2 Feb 21, 2017

What happened to -105? Was it previously already used? Or is it part of code Yahoo intends to push back?

This comment has been minimized.

Copy link
@knusbaum

knusbaum Feb 21, 2017

Author Contributor

Part of our pushback. I wasn't sure how best to handle the discrepancy. Mixing them up now seems undesirable.

This comment has been minimized.

Copy link
@revans2

revans2 Feb 21, 2017

Can we at least put in a comment about it?

@knusbaum

This comment has been minimized.

Copy link
Contributor Author

knusbaum commented Feb 23, 2017

@reddycharan Do you see any problems with this?

@jvrao

This comment has been minimized.

Copy link
Contributor

jvrao commented Feb 24, 2017

Couple of questions:

  1. Charan with this change how does LongHierarchicalLedgerManager users (Salesforce) has backward compatibility?
  2. @knusbaum Why not keep LongHierarchicalLedgerManager around? even when HierarchicalLedgerManager supporting both ids.?
@knusbaum

This comment has been minimized.

Copy link
Contributor Author

knusbaum commented Feb 24, 2017

@jvrao @reddycharan

So my plan is this: Keep LongHierarchicalLedgerManager as is currently in master, move HierarchicalLedgerManager to LegacyHierarchicalLedgerManager and create the new backwards-compatible one as HierarchicalLedgerManager. This way, if you're already using LongHierarchicalLedgerManager, it will still work. If you're using the current HierarchicalLedgerManager, that will work, and if you (for some unknown reason) want to use the old legacy one, it still exists as well.

Thoughts?

@reddycharan

This comment has been minimized.

Copy link
Contributor

reddycharan commented Feb 24, 2017

It sounds good that LongHLM is not changed. For HierarchicalLM also, it sounds good, but I'm not the best person to comment since we are not using it. Thanks @knusbaum.

@knusbaum

This comment has been minimized.

Copy link
Contributor Author

knusbaum commented Feb 27, 2017

Closing in favor of #114

@knusbaum knusbaum closed this Feb 27, 2017
asfgit pushed a commit that referenced this pull request May 2, 2017
This PR supersedes #112

Instead of moving LongHierarchicalLedgerManager to HierarchicalLedgerManager, LongHierarchicalLedgerManager is still a stand-alone manager. HierarchicalLedgerManager has been moved to LegacyHierarchicalLedgerManager, and a new HierarchicalLedgerManager class has been put in its place, which is backwards-compatible with the original (now legacy) HierarchicalLedgerManager.

This new HierarchicalLedgerManager leverages the new LongZkLedgerIdGenerator to generate Ids, and uses the LongHierarchicalLedgerManager to manage metadata for ledger IDs > 31 bits long. For shorter ledger ids, the LegacyHierarchicalLedgerManager is used, to keep backwards-compatibility.

Author: Kyle Nusbaum <knusbaum@yahoo-inc.com>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Matteo Merli <mmerli@apache.org>, Venkateswararao Jujjuri (JV) <None>

Closes #114 from knusbaum/BOOKKEEPER-552
athanatos pushed a commit to athanatos/bookkeeper that referenced this pull request Jan 25, 2019
…le reading the proposal packet

ZOOKEEPER-2355:Ephemeral node is never deleted if follower fails while reading the proposal packet

Author: arshadmohammad <arshad.mohammad.k@gmail.com>
Author: Mohammad Arshad <arshad@apache.org>

Reviewers: Michael Han <hanm@apache.org>, Rakesh Radhakrishnan <rakeshr@apache.org>, Flavio Junqueira <fpj@apache.org>, Jiang Jiafu <jiangjiafu1989@gmail.com>

Closes apache#112 from arshadmohammad/ZOOKEEPER-2355
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.