Use readExplicitLAC instead of readEntry in order to get current LastAddConfirmed by eolivelli · Pull Request #1572 · apache/bookkeeper

eolivelli · 2018-07-30T11:27:59Z

Motivation

Readers have to explicitly call readExplicitLastConfirmed in order to leverage ExplicitLAC.
I would like to have a unified way to read LastAddConfirmed.

Changes

This change makes the client use internally readExplicitLastConfirmed instead of using readLastAddConfirmed.
We need to introduce something similar to tryReadLastAddConfirmed in order to make it consistent with readLastAddConfirmed.

It lacks support for Long Poll Reads, notably we would have to listen on ExplicitLAC modifications on the bookie side in order to wake up the readers even in case of setExplicitLAC from the writer and we would need to change protocol for readLastAddConfirmedAndEntry in order to return the ExplicitLAC.

We cannot support v2 clients.

eolivelli · 2018-08-18T11:06:35Z

@sijie @jvrao can you take a look to this idea?

eolivelli · 2018-08-22T08:32:22Z

@sijie @jvrao can you take a look to this idea?

sijie · 2018-08-22T09:28:06Z

@eolivelli sorry I missed your comment. I typically ignore any WIP PRs...

sijie

the approach seems to be okay to me.

one general comment: why it is only applied to asyncTryReadLastConfirmed?

eolivelli · 2018-08-22T13:49:29Z

-            };
-
-        new ReadLastConfirmedOp(this, innercb).initiate();
+        asyncReadExplicitLastConfirmed(cb, ctx);


@sijie here I am changing also asyncReadLastConfirmed

This is actually the core of the change.
I had to re implement asyncTryReadLastConfirmed because there is no existing support for explicitLAC

…AddConfirmed

eolivelli · 2018-12-28T08:39:56Z

@sijie @jvrao @ivankelly I have rebased this patch onto current master.
I would like to insert it in 4.9.

I only have to add unit tests. All current tests about readLastAddConfirmed() and tryReadLastAddConfirmed() are passing even by enabling the feature by default.
So I only have to find interesting tests and transform them to a parametrized version which run the test with/without this new feature.

sijie · 2018-12-28T18:38:40Z

+        }
+    }
+
+    private void asyncReadLastConfirmedInternal(final ReadLastConfirmedCallback cb, final Object ctx) {


asyncReadLastConfirmedInternal is a bad name. I would prefer calling it asyncReadPiggybackLastConfirmed (in contrast to asyncReadExplicitLastConfirmed.

sijie · 2018-12-28T18:41:00Z

     */

    public void asyncReadLastConfirmed(final ReadLastConfirmedCallback cb, final Object ctx) {
+        if (clientCtx.getConf().useExplicitLacForReads) {


I am not sure using a flag is the best choice here. the bookkeeper client should be smart to figure out whether it should be reading explicit lac or piggybacked lac.

Maybe the name is misleading but it is already here.
This function will compose explicit and piggybacked LAC.
So actually it would be better to use always readExplicitLAC RPC, but we cannot enable it by default because it works only with recent bookies and with v3 protocol

Client knows it is using V3 protocol or not; so why not use the explicitLAC for >= v3 automatically without a conf option?

ExplicitLAC is since 4.5. V3 protocol was already present. So we have bookies which talk v3 and do not know ExplicitLAC protocol: we will break compatibility from 4.9 clients and 4.4 bookies.
Honestly I don't think this is a real production problem because 4.4 is very old, but we don't have a clear EOL policy.

@eolivelli I don't understand your comment here. if a bookie doesn't know ExplicitLAC, the request will be rejected and the client will know about that, right? when client know that, it can fallback to disable using explicit lac, no?

@sijie so you are suggesting to fallback to piggybackLac in case of "UnsupportedOperation" error from bookie.
We should keep track of this per-bookie. I think this is very tricky, we need to add a lot of code and runtime overhead for the sake of compatibility with very old bookies (4.4).

We can have this configuration flag, enabled by default in 4.9 in case of v3+ protocol and disable for v2 protocol, and then drop it as soon as we decide to drop support for compatibility with such old versions.

@eolivelli

you are suggesting to fallback to piggybackLac in case of "UnupportedOperation" error from bookie.

yes.

We should keep track of this per-bookie.

probably yes.

We can have this configuration flag, enabled by default in 4.9 in case of v3+ protocol and disable for v2 protocol,

my main concern is with this change, we have so many different variants of reading lac operations in the main repo. it is very confusing and hard to maintain. I would suggest us holding on merging this PR and thinking of a better solution for it, rather than rushing at merging this and make things hard to maintain in future.

@sijie I see your concern. There is no hurry, we can work on this topic for 4.10.
I think that current asyncReadExplicitLastConfirmed() is better because it is actually merging the two values (piggy backed + explicit)

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadLacOp.java

Line 115 in e426547

if (lastEntryBuffer != null && lastEntryBuffer.readableBytes() > 0) {

I think this is the future, I am not dropping the legacy method only for compatibility with 4.4.

The best would be to not have the flag, not perform any compatibility effort, and say that 4.9 clients are able to read from 4.4 bookies only using v2 protocol (in v2 protocol we don't have ExplicitLAC so we cannot use PendingReadLacOp)

PendingReadLacOp is already here, I am not adding it, I am only suggesting to use PendingReadLacOp for regular readLastAddConfirmed() instead of ReadLastConfirmedOp.

Ideally we should merge PendingReadLacOp with ReadLastConfirmedOp and drop PendingReadLacOp, but it is greater work, because ReadLastConfirmedOp handles fencing stuff

jvrao · 2018-12-29T01:32:57Z

+                // Extract lac from last entry on the disk
+                if (lastEntryBuffer != null && lastEntryBuffer.readableBytes() > 0) {
+                    RecoveryData recoveryData = lh.macManager.verifyDigestAndReturnLastConfirmed(lastEntryBuffer);
+                    long recoveredLac = recoveryData.getLastAddConfirmed();


isn't it called piggyBackedLAC?

renamed to newLac, it is clearer, thanks

jvrao · 2018-12-29T01:41:16Z

+
+/**
+ * This op is try to read last confirmed without involving quorum coverage checking.
+ * Use {@link PendingLacOp} if you need quorum coverage checking.


You mean PendingReadLacOp?

I am not sure if we ever need quorum for LAC. LAC means it has quorum, so we can take greatest LAC.

If we take out the coverageSet.checkCovered() check from PendingReadLacOp, then we don't need this right? I don't know why we have the quorum check in there, What scenario needs LAC to be quorum checked? @sijie ?

Haven't looked at this patch, but this coverage check is there because fencing is piggybacked on the LAC read.

@jvrao @ivankelly @sijie
This feature has the same semantics as TryReadLastConfirmedOp:

TryPendingReadLacOp -> PendingReadLacOp
TryReadLastConfirmedOp -> ReadLastConfirmedOp

We are trying to advance LAC and return as soon as we have an useful value, without full coverage checks.

AFAIK This feature is a used for Long-Poll reads, which I am not going to implement soon, at least not within 4.9 timeline.
In Long Poll reads tests I have found this API asyncTryReadLastConfirmed and for consistency I have created a similar one for ExplicitLAC based reads.

without full coverage checks.

This is what I am trying to understand. Once we have LAC updated (either piggy-back or explicit) it means that it has met Write Quorum. So I don't understand what is the additional coverage check needed. This is not a fencing read or reading beyond LAC, this operation reads LAC which must have been updated by the writer after write quorum, hence it is persisted. If we need latest LAC we need quorum (for fencing) but for reads, you don't need.

jvrao · 2018-12-29T01:47:57Z

+                    RecoveryData recoveryData = lh.macManager.verifyDigestAndReturnLastConfirmed(lastEntryBuffer);
+                    long recoveredLac = recoveryData.getLastAddConfirmed();
+                    if (recoveredLac > maxLac) {
+                        newLac = recoveredLac;


You are overwriting newLac.

good catch!

eolivelli · 2018-12-31T12:09:55Z

For what is worth...
I am testing this patch with some projects and it works like a charm, on the writers you can enable ExplicitLAC and the readers are able to see the LAC advance without changing code (which is using readLastAddConfirmed and not readExplicitLastConfirmed).
No need to extra NOOP writes to make LAC advance, all is transparent.

I think this is a great step forward for BK usability

eolivelli · 2019-01-06T21:57:19Z

Ping @sijie

sijie

I still don't understand why we can't merge explicit lac and piggybacked lac into one read operations. my concern is after this change, we ended up having many variants of reading last confirmed. what is your plan for that in long term?

sijie · 2019-01-08T12:01:44Z

     */

    public void asyncReadLastConfirmed(final ReadLastConfirmedCallback cb, final Object ctx) {
+        if (clientCtx.getConf().useExplicitLacForReads) {


@eolivelli I don't understand your comment here. if a bookie doesn't know ExplicitLAC, the request will be rejected and the client will know about that, right? when client know that, it can fallback to disable using explicit lac, no?

sijie · 2019-01-08T12:02:20Z

+        if (clientCtx.getConf().useExplicitLacForReads) {
+            asyncTryReadExplicitLastConfirmed(cb, ctx);
+        } else {
+            asyncTryReadLastConfirmedInternal(cb, ctx);


Why the name here is not consistent with asyncReadPiggybackLastConfirmed?

@sijie I missed to renamed this method as well.
done

eolivelli · 2019-01-11T22:34:46Z

@sijie @jvrao @ivankelly
I am closing this one in favour of #1901 which is simpler and does not introduce new variants of read LAC.
We will deal with tryReadLastAddConfirmed in the future.
To me it is important to enable new API users to use ExplicitLAC

eolivelli requested a review from sijie July 30, 2018 11:27

eolivelli force-pushed the fix/explicitlac-reader branch from 5918517 to 5f5ed5c Compare July 30, 2018 11:29

eolivelli requested a review from jvrao July 31, 2018 08:27

sijie reviewed Aug 22, 2018

View reviewed changes

eolivelli commented Aug 22, 2018

View reviewed changes

eolivelli added 3 commits December 28, 2018 08:46

Use readExplicitLAC instead of readEntry in order to get current Last…

350c872

…AddConfirmed

fix checkstyle

7a352bb

Rebases to latest master and add new configuration parameter

0872243

eolivelli force-pushed the fix/explicitlac-reader branch from 42b5a25 to 0872243 Compare December 28, 2018 08:35

eolivelli changed the title ~~WIP - Use readExplicitLAC instead of readEntry in order to get current LastAddConfirmed~~ Use readExplicitLAC instead of readEntry in order to get current LastAddConfirmed Dec 28, 2018

sijie assigned eolivelli Dec 28, 2018

sijie added type/feature area/client labels Dec 28, 2018

sijie requested changes Dec 28, 2018

View reviewed changes

jvrao reviewed Dec 29, 2018

View reviewed changes

Address comments and add tests

49914bf

Fix checkstyle

6988b16

sijie reviewed Jan 8, 2019

View reviewed changes

Rename method

bf948ad

eolivelli closed this Jan 11, 2019

Uh oh!

Conversation

eolivelli commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

eolivelli commented Aug 18, 2018

Uh oh!

eolivelli commented Aug 22, 2018

Uh oh!

sijie commented Aug 22, 2018

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvrao Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Dec 31, 2018

Uh oh!

eolivelli commented Jan 6, 2019

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Jan 11, 2019

Uh oh!

eolivelli commented Jul 30, 2018 •

edited

Loading

eolivelli commented Dec 28, 2018 •

edited

Loading

eolivelli Jan 8, 2019 •

edited

Loading

jvrao Jan 3, 2019 •

edited

Loading