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

make ledger metadata immutable #281

Closed
sijie opened this issue Jul 24, 2017 · 6 comments
Closed

make ledger metadata immutable #281

sijie opened this issue Jul 24, 2017 · 6 comments

Comments

@sijie
Copy link
Member

sijie commented Jul 24, 2017

FEATURE REQUEST

  1. Please describe the feature you are requesting.

Address the TODO item in LedgerHandle

            if (isClosed()) {
                // TODO: make ledger metadata immutable
                // Although the metadata is already closed, we don't need to proceed zookeeper metadata update, but
                // we still need to error out the pending add ops.
                //
                // There is a race condition a pending add op is enqueued, after a close op reset ledger metadata state
                // to unclosed to resolve metadata conflicts. If we don't error out these pending add ops, they would be
                // leak and never callback.
                //
                // The race condition happen in following sequence:
                // a) ledger L is fenced
                // b) write entry E encountered LedgerFencedException, trigger ledger close procedure
                // c) ledger close encountered metadata version exception and set ledger metadata back to open
                // d) writer tries to write entry E+1, since ledger metadata is still open (reset by c))
                // e) the close procedure in c) resolved the metadata conflicts and set ledger metadata to closed
                // f) writing entry E+1 encountered LedgerFencedException which will enter ledger close procedure
                // g) it would find that ledger metadata is closed, then it callbacks immediately without erroring out any pendings
                synchronized (LedgerHandle.this) {
                    pendingAdds = drainPendingAddsToErrorOut();
                }
                errorOutPendingAdds(rc, pendingAdds);
                cb.closeComplete(BKException.Code.OK, LedgerHandle.this, ctx);
                return;
            }
  1. Indicate the importance of this issue to you (blocker, must-have, should-have, nice-to-have). Are you currently using any workarounds to address this issue?

nice-to-have

  1. Provide any additional detail on your proposed use case for this feature.
@sijie
Copy link
Member Author

sijie commented Nov 22, 2017

@ivankelly @jiazhai I marked this for 4.7.0 now. there have been discussions and concerns around the mutability of ledger metadata. let's pick this up in 4.7.0.

@sijie
Copy link
Member Author

sijie commented Feb 1, 2018

I don't think we are able to do this in any time soon. dropping the release version at this moment.

@sijie sijie removed this from the 4.7.0 milestone Feb 1, 2018
@ivankelly
Copy link
Contributor

ya, not a hope for 4.7.0. This needs yahoo and saleforce merge to be 100% complete. We're close, but better to postpone til 4.8.0.

I remember when I did it last time, the change only took about a day.

@ivankelly ivankelly added the triage/18-w11 Issue triage, 2018 week 11 label Mar 12, 2018
@ivankelly
Copy link
Contributor

@reddycharan @jvrao Are salesforce planning to make any changes to the client metadata handling?

ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Jul 31, 2018
First part of changes to make ledger metadata immutable. The client
should only act on metadata which has been written to zookeeper. To
this end, we need to be able to get back from the ledger manager what
has been written to zookeeper (with the version number updated).

Master issue: apache#281
ivankelly added a commit that referenced this issue Jul 31, 2018
First part of changes to make ledger metadata immutable. The client
should only act on metadata which has been written to zookeeper. To
this end, we need to be able to get back from the ledger manager what
has been written to zookeeper (with the version number updated).

Master issue: #281

Author: Ivan Kelly <ivan@ivankelly.net>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1573 from ivankelly/ledger-manager-returns-ledgermeta
ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Jul 31, 2018
This object has been accessed and mutated all over the client, which
makes it hard to do anything with the object. This patch removes the
direct accesses, so the object can only be accessed through an
accessor.

Master issue: apache#281
ivankelly added a commit that referenced this issue Jul 31, 2018
This object has been accessed and mutated all over the client, which
makes it hard to do anything with the object. This patch removes the
direct accesses, so the object can only be accessed through an
accessor.

Master issue: #281

Author: Ivan Kelly <ivan@ivankelly.net>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1574 from ivankelly/onlyaccessmdthroughaccessor
ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Jul 31, 2018
Previously, the ensemble list was a Map<Long, ArrayList<BookieSocketAddress>>.
ArrayList is by definition mutable, so ensemble passed to metadata users are
always mutable.

This patch changes in ensembles in the list to be immutable. We were also leaking
the implementation of ledger metadata to the placement policy, so this has been
modified to use List<BookieSocketAddress> also.

Master issue: apache#281
ivankelly added a commit that referenced this issue Aug 1, 2018
Previously, the ensemble list was a Map<Long, ArrayList<BookieSocketAddress>>.
ArrayList is by definition mutable, so ensemble passed to metadata users are
always mutable.

This patch changes in ensembles in the list to be immutable. We were also leaking
the implementation of ledger metadata to the placement policy, so this has been
modified to use List<BookieSocketAddress> also.

Master issue: #281

Author: Ivan Kelly <ivan@ivankelly.net>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org>

This closes #1575 from ivankelly/ledger-fragment-immutable-metadata and squashes the following commits:

14977a2 [Ivan Kelly] fix dlog
bdaef31 [Ivan Kelly] checkstyle
6e34311 [Ivan Kelly] Make each ensemble in ensemble list immutable
ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Aug 2, 2018
This patch introduces a metadata update mechanism for the client which
will be used in all places where metadata is updated.

The mechanism takes a bunch of predicates and functions, and runs a
loop again the ledger manager, attempting to apply the mutation
required as specified.

It assumes that the ledger metadata objects on the client side are
immutable and that any metadata object read reflects state that exists
on the metadata store. This isn't the case right now, but as the
current metadata updates are changed to use this, it will be the case.

This patch also introduces a limited LedgerMetadataBuilder, though
only the fields required for testing at mutable.

Master Issue: apache#281
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 22, 2018
LedgerMetadata shouldn't generally be created outside the client,
except for testing. In the case where it is, the builder should be
used to construct it.

I've left the copy constructor for now, as I don't feel comfortable
removing it until all fields are final, which will occur soon.

Master issue: apache#281
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 22, 2018
Make most fields in LedgerMetadata final. The ones which I have not
changed in this patch require a more involved change, so they'll come
in separate PRs.

Master issue: apache#281
ivankelly added a commit that referenced this issue Nov 22, 2018
LedgerMetadata shouldn't generally be created outside the client,
except for testing. In the case where it is, the builder should be
used to construct it.

I've left the copy constructor for now, as I don't feel comfortable
removing it until all fields are final, which will occur soon.

Master issue: #281


Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1827 from ivankelly/kill-constructors
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 22, 2018
Make most fields in LedgerMetadata final. The ones which I have not
changed in this patch require a more involved change, so they'll come
in separate PRs.

Master issue: apache#281
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 22, 2018
The method is only used in the test, so it should live there.

Master issue: apache#281
ivankelly added a commit that referenced this issue Nov 22, 2018
Make most fields in LedgerMetadata final. The ones which I have not
changed in this patch require a more involved change, so they'll come
in separate PRs.

Master issue: #281


Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1828 from ivankelly/mostly-final
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 22, 2018
Remove any methods in LedgerMetadata that modify the ensemble. With
this change the ensembles are 100% immutable.

Master issue: apache#281
sijie pushed a commit that referenced this issue Nov 22, 2018
The method is only used in the test, so it should live there.

Master issue: #281


Reviewers: Sijie Guo <sijie@apache.org>, Enrico Olivelli <eolivelli@gmail.com>

This closes #1829 from ivankelly/move-junk
ivankelly added a commit that referenced this issue Nov 23, 2018
Remove any methods in LedgerMetadata that modify the ensemble. With
this change the ensembles are 100% immutable.

Master issue: #281


Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org>

This closes #1830 from ivankelly/no-mod-ensemble
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 23, 2018
ivankelly added a commit that referenced this issue Nov 23, 2018
Master issue: #281


Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org>

This closes #1831 from ivankelly/close-immut
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 23, 2018
A copy constructor makes no sense when it's not possible to mutate the object.

Master issue: apache#281
ivankelly pushed a commit to ivankelly/bookkeeper that referenced this issue Nov 23, 2018
ivankelly added a commit that referenced this issue Nov 23, 2018
A copy constructor makes no sense when it's not possible to mutate the object.

Master issue: #281


Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1832 from ivankelly/kill-copy
ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Nov 23, 2018
The two parameters are long, which can be confusing.

Master issue: apache#281
ivankelly added a commit to ivankelly/bookkeeper that referenced this issue Nov 23, 2018
There's never a case when setting the digest and password where you
only have one, but not the other. The builder should reflect this.

Master issue: apache#281
ivankelly added a commit that referenced this issue Nov 23, 2018
Master issue: #281


Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #1833 from ivankelly/immut-cust
ivankelly added a commit that referenced this issue Nov 29, 2018
* Rename LedgerMetadataBuilder#closingAt to clarify parameter order

The two parameters are long, which can be confusing.

Master issue: #281
@ivankelly
Copy link
Contributor

The metadata is immutable now. See the long list of changes for details.

sijie pushed a commit that referenced this issue Dec 1, 2018
There's never a case when setting the digest and password where you
only have one, but not the other.

The patch adds validation in LedgerMetadata that if you provide one, you
provide the other. The only case where you don't provide these is when 
using pre-4.1.0 (i think) metadata.

Master issue: #281


Reviewers: Sijie Guo <sijie@apache.org>, Enrico Olivelli <eolivelli@gmail.com>

This closes #1836 from ivankelly/builder-pw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants