On Tue, Feb 6, 2018 at 11:00 PM, Sijie Guo <guosi...@gmail.com> wrote:
> On Tue, Feb 6, 2018 at 9:37 PM Venkateswara Rao Jujjuri <jujj...@gmail.com
> >
> wrote:
>
> > We have bunch of code in doAsyncCloseInternal() to handle
> > MetadataVersionException.
> > I am wondering if there is a valid case where we need to handle this.
> Stay
> > with me. :)
> >
> > When do we get this error?
> >
> > 1. When the client tried to write the metadata twice. Same metadata twice
> > because of the client tried twice and the first attempt made it. (retry
> > logic in bk). If so, the error is benign. (Though I have a question on
> > metadata.isNewerThan(newMeta) logic which I will ask towards the end.)
> >
> > 2. Close might be happening for two reasons. 1. Application initiated 2.
> > Internal initiated because of a hard error on the write path. We really
> > don't need to fail with version mismatch for internal closes, as it
> really
> > doesn't matter, because our intention is to fail the write and we would
> > anyway. Just close the local handle we are done.
> > In the case of the application initiated close, the version mismatch can
> > happen only if the fencing logic took over and changed some metadata. But
> > we are guaranteed to protected up until the LAC of the writer anyway.
> > Since the leger is getting closed, it really doesn't matter if the ZK has
> > more updated metadata version.
> >
> > So my question is, do we really need to handle MetadataVersionException
> on
> > write?
> >
> > My next question is do we have anything fishy in the isNewerThan logic?
> >
> > boolean isNewerThan(LedgerMetadata newMeta) {
> > if (null == version) {
> > return false;
> > }
> > return Version.Occurred.AFTER == version.compare(newMeta.
> version);
> > }
> > ---
> > MetadataVersion mv = (MetadataVersion) v;
> > int res = version - mv.version;
> > if (res == 0) {
> > return Occurred.CONCURRENTLY;
> > } else if (res < 0) {
> > return Occurred.BEFORE;
> > } else {
> > return Occurred.AFTER;
> > }
> > ---
> >
> > Since newMetadata is newer in this case, its version is going to be
> higher
> > hence res is -ve, and this is returning Occurred.BEFORE, but it must be
> > Occurred.AFTER as the metadata update happened 'after' what we have. Or
> the
> > isNewerThan logic is inverted.
> >
> > if (!metadata.isNewerThan(newMeta)
> > && !metadata.isConflictWith(newMeta)) {
> > } else {
> > LOG.warn("Conditional update ledger metadata for ledger {} failed.",
> > ledgerId);
> > cb.closeComplete(rc,
> > LedgerHandle.this, ctx);
> > }
> >
> > In this case, even though local metadata is NOT newer than newMeta
> because
> > of the logic we are failing.
> >
> >
> > So two questions:
> > 1. Do we really need to handle MetadataVersionMismatch in close?
>
>
> I think the take was: when you got metadata version mismatch, you don’t
> really know who updated the metadata, could be another writer, could be
> retried from same writer. So we popped up the error to application and let
> applications decide what to do. I think this model works fine, since it is
> on closing path, application can choose ignore close error. Do you see any
> bad effects on your applications?
>
>
I just want to check if there is anything we need to be worried about
if application/client ignores version mismatch error on close. Looks like
its ok. Right?
>
> > 2. Is this metadata.isNewerTha() logic is corretc?
> >
> >
> The logic seems to be fine with me. A.isNewerThan(B), is checking if A’s
> version is after B’s version, no?
>
>
I got my logic inverted; never mind.
> Sijie
>
>
> --
> > Jvrao
> > ---
> > First they ignore you, then they laugh at you, then they fight you, then
> > you win. - Mahatma Gandhi
> >
>
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
