You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Venkateswara Rao Jujjuri <ju...@gmail.com> on 2018/02/07 05:37:25 UTC
MetadataVersionException handling in doAsyncCloseInternal()
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?
2. Is this metadata.isNewerTha() logic is corretc?
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
Re: MetadataVersionException handling in doAsyncCloseInternal()
Posted by Sijie Guo <gu...@gmail.com>.
On Wed, Feb 7, 2018 at 11:30 PM, Venkateswara Rao Jujjuri <jujjuri@gmail.com
> wrote:
> On Tue, Feb 6, 2018 at 11:00 PM, Sijie Guo <gu...@gmail.com> wrote:
>
> > On Tue, Feb 6, 2018 at 9:37 PM Venkateswara Rao Jujjuri <
> jujjuri@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?
>
Yeah that's okay
>
> >
> > > 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
>
Re: MetadataVersionException handling in doAsyncCloseInternal()
Posted by Venkateswara Rao Jujjuri <ju...@gmail.com>.
On Tue, Feb 6, 2018 at 11:00 PM, Sijie Guo <gu...@gmail.com> wrote:
> On Tue, Feb 6, 2018 at 9:37 PM Venkateswara Rao Jujjuri <jujjuri@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
Re: MetadataVersionException handling in doAsyncCloseInternal()
Posted by Sijie Guo <gu...@gmail.com>.
On Tue, Feb 6, 2018 at 9:37 PM Venkateswara Rao Jujjuri <ju...@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?
> 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?
Sijie
--
> Jvrao
> ---
> First they ignore you, then they laugh at you, then they fight you, then
> you win. - Mahatma Gandhi
>