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?
> 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
>
