BP-14 force() API - client side implementation #1436

eolivelli · 2018-05-24T15:20:25Z

Introduce the client side force() API
Implementation on the client side wire protocol for FORCE_LEDGER RPC
Disable ensemble changes for DEFERRED_SYNC writers
Prevent v2 client from using force() API.

The force() API enables the client (usually with DEFERRED_SYNC write flags) to require a point of synchronization with all the bookies in the ensemble, to have guarantees about durability of previously written entries (and ackknowledgerd), this way LastAddConfirmed is able to advance.

For DEFERRED_SYNC writers LastAddConfirmed will advance only using this API

eolivelli · 2018-05-27T14:51:18Z

@jvrao @sijie the patch is ready for review now

eolivelli · 2018-05-28T06:16:18Z

retest this please

eolivelli · 2018-05-28T15:29:28Z

retest this please

jiazhai · 2018-05-29T02:13:10Z

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

@@ -1790,6 +1851,14 @@ void handleDelayedWriteBookieFailure() {
            }
            return;
        }
+        if (writeFlags.contains(WriteFlag.DEFERRED_SYNC)) {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Cannot perform ensemble changee with writeflags {}."


a typo changee

jiazhai · 2018-05-29T02:20:56Z

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

@@ -41,6 +41,7 @@

    String ADD_OP = "ADD_ENTRY";
    String ADD_OP_UR = "ADD_ENTRY_UR"; // Under Replicated during AddEntry.
+    String FORCE_OP = "FORCE";


how about also add some comments for force here?

jiazhai · 2018-05-29T02:42:38Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java

+            long ledgerId = forceLedgerResponse.getLedgerId();
+
+            if (LOG.isDebugEnabled()) {
+                logResponse(status, "ledger", ledgerId);


seems logResponse already has an isDebugEnabled() internal.

this is consistent with all the rest of the file, I can change every where, but I think that it can be a slight save of resources.
I would prefer to keep the style consistent.
I will change if you feel strong about it

sijie · 2018-05-29T03:05:11Z

@jvrao @eolivelli : I am wondering how would be the relationship between force() and explicitLac. my feeling is that these two operations are kind of similar/same operation, basically we are requesting advancing lac in either case.

If you guys agree with that, it is probably making more sense to reuse explicitLac as the request for force() api. we can add a flag in explicitLac indicating whether we want to force the journal.

if no, we need to figure out what does following behavior mean:

if a client enables explicit lac, what explicitLac is exactly doing for deferred sync ledgers.

eolivelli · 2018-05-29T04:36:54Z

@sijie explicitlac for deferred sync writers only sends the value of LastAddConfirmed and it is still a way to notify LAC changes without a real write. The difference is that LastAddConfirmed variable does not advance at every write.

I think they are different operations, and need different semantics. Force() must be acknowledged by the full ensemble.
You can use force() without sending explicitlac, in fact new LAC will be piggybacked at next write for instance.

I think it is better to keep force and explicitlac separated.

I will add test cases to cover the usage of explicit LAC with deferred sync flag.

I will check the implementation today, in case I am missing something.

Cc @jvrao

jvrao · 2018-05-29T05:49:50Z

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

+        // remember that we are inside OrderedExecutor, this induces a strict ordering
+        // on the sequence of events
+        this.currentNonDurableLastAddConfirmed = lh.pendingAddsSequenceHead;
+        LOG.debug("start force() on ledger {} capturing {} ", lh.ledgerId, currentNonDurableLastAddConfirmed);


instead of capturing {} ; just say clientNonDurableLac {}

jvrao · 2018-05-29T05:53:10Z

...keeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java

@@ -60,6 +60,11 @@ public WriteSet getWriteSetForLongPoll(long entryId) {
        return WriteSetImpl.create(ensembleSize, ensembleSize /* writeQuorumSize */, entryId);
    }

+    @Override
+    public WriteSet getWriteSetForForceLedger() {
+        return WriteSetImpl.create(ensembleSize, ensembleSize /* writeQuorumSize */, 0);


Have a comment similar to longPoll method above.

jvrao · 2018-05-29T06:03:44Z

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

+                bk.getMainWorkerPool().submit(new SafeRunnable() {
+                    @Override
+                    public void safeRun() {
+                        LOG.warn("Attempt to use a closed ledger: {}", ledgerId);


Nit: Force() attempted on a closed ledger {}

jvrao · 2018-05-29T06:05:02Z

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

+            return result;
+        }
+
+        if (pendingAddsSequenceHead == INVALID_ENTRY_ID) {


Please add comments on what it really means.

jvrao · 2018-05-29T06:10:59Z

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

+                LOG.debug("Cannot perform ensemble changee with writeflags {}."
+                        + "Failed bookies {} for ledger {}.",
+                        writeFlags, delayedWriteFailedBookies, ledgerId);
+            }


Maybe we should treat this also unrecoverable error, or even fail at the caller itself.
Because if AQ< WQ we can continue to write but force is going to fail anyway.
To be consistent, any write error (even if it is delayed write error it needs to fail under this flag)

actually with writeflag = DEFERRED_SYNC this block of code will never run.
I have copied this block for consistency with other cases of "enabled" disableEnsembleChangeFeature.

I can drop it if you prefer, this code will never run

jvrao · 2018-05-29T06:13:47Z

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

@@ -284,7 +284,9 @@ public void writeComplete(int rc, long ledgerId, long entryId, BookieSocketAddre
                // Got an error after satisfying AQ. This means we are under replicated at the create itself.
                // Update the stat to reflect it.
                addOpUrCounter.inc();
-                if (!lh.bk.getDisableEnsembleChangeFeature().isAvailable() && !lh.bk.delayEnsembleChange) {
+                if (!lh.bk.getDisableEnsembleChangeFeature().isAvailable()
+                        && !lh.writeFlags.contains(WriteFlag.DEFERRED_SYNC)


What is the use of adding this check here?

We are acting as "enabled" disableEnsembleChangeFeature, so I kept the same exact beheaviour

I don't think this check makes sense. This may not cause harm as failedBookies set is best effort scenario.
@sijie do you think we need this check here?

sijie · 2018-05-29T06:13:55Z

I think they are different operations, and need different semantics. Force() must be acknowledged by the full ensemble.
You can use force() without sending explicitlac, in fact new LAC will be piggybacked at next write for instance.

I am not saying they are same operations. what I am questioning here is - is it better to extend the explicit lac request with a flag that force flushing the data? In that way the behavior would be better defined when people enable explicit lac on a deferred-sync ledger.

Or we have to clarify following cases:

normal ledger, without explicit lac
normal ledger, with explicit lac
deferred ledger, without explicit lac
deferred ledger, with explicit lac

The case 4 is the most confused case.

jvrao · 2018-05-29T06:35:49Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java

+        // Build the request
+        BKPacketHeader.Builder headerBuilder = BKPacketHeader.newBuilder()
+                .setVersion(ProtocolVersion.VERSION_THREE)
+                .setOperation(OperationType.FORCE_LEDGER)


Did we make protobuf changes for this?

There is a new RPC for FORCE_LEDGER, nothing else

jvrao · 2018-05-29T06:39:32Z

Or we have to clarify following cases

I don't understand where the explicitLAC is clashing with the deferred_sync. ExplicitLAC is completely independent operatin, where if the client thinks it's LAC is ahead of piggybacked LAC, it can send an update. and differedSync dictates how the client LAC gets updated. Maybe I am missing something @sijie

sijie · 2018-05-29T07:31:42Z

don't understand where the explicitLAC is clashing with the deferred_sync.

@jvrao : explicit lac is advancing lac, while force is also advancing lac. my point is why can we consolidate these two behaviors as one. to me, 'explicit lac' can be a background 'force'; 'force' is an explicit application-defined 'explict lac' operation.

eolivelli · 2018-05-29T14:31:40Z

@jiazhai @jvrao I have addressed all of your comments and provided answers.

eolivelli · 2018-05-29T14:41:54Z

@sijie I have added a test case about using ExplicitLAC and DEFERRED_SYNC.

I think that current behaviour is what I am expecting:

LastAddConfirmed advances only in case of force()
ExplicitLAC timer sends notifications only when LastAddConfirmed, in case of DEFERRED_SYNC this is possible only after force()
without force() ExplicitLAC does nothing, as expected.

It seems to me that this is consistent with the expectations of DEFERRED_SYNC writeflag.

is it better to extend the explicit lac request with a flag that force flushing the data? In that way the behavior would be better defined when people enable explicit lac on a deferred-sync ledger.

I think that the client controls LAC using force() and ExplicitLAC is a background task which seems to me like an optimization.
I don't see much benefit in having an "implicit" force with the ExplicitLAC notification.

Maybe it is like the initial proposal of having a "piggybacked LastAddPersisted" on AddResponse which would have made LAC to advance even without an explicit "force()".

We could add a flag which enables a timer to send explicit force() periodically, or that performs a force() during the execution of ExplicitLAC timer.
I prefer to keep things separated at wire protocol, we can compose the two RPCs, they are lightweight and we are in a background task (the ExplicitLAC timer)

cc @jvrao

sijie · 2018-05-29T17:54:22Z

@eolivelli @jvrao :

if we step back to think about the behavior here, both of these two features are kind of "flush/sync" behavior. forget about the implementation here, think from a storage system, image we have a "flush" primitive - it basically flushes unwritten data and make these data available.

so,

"explicit lac" - it is a background "flush", flushing the data between lac and lastAddPushed. it is running in background.
"force" - it is an explicit "flush", flushing the data between current lac and lastAddPushed, it is explicitly.

those are exactly similar/same behavior from this sense.

if you look at filesystem, filesystem has pdflush doing background flush and provides fsync systemcall for applications to do explicitly flush. but under the hood, they are doing the same thing - flushing the unflushed data.

if you map the behavior of bookkeeper to filesystem, "explicit lac" is exactly the background flush "pdflush"; while "force" is the system call.

my main point here is if these two behaviors are similar/same, we should consolidate the underlying mechanism rather than having two separate routes to do same operations.

eolivelli · 2018-05-29T19:23:02Z

@sijie your point is very interesting.
Let me think about it a bit more.

sijie · 2018-06-12T06:45:38Z

in the fsync / force metaphor I think it is better not to fail pending adds.

sorry your comment isn't clear to you. are you saying JV asked for failing pending adds when encountered issues on #force? or not failing?

eolivelli · 2018-06-12T12:27:47Z

I meant @jvrao suggested to error our pending adds.
I think we do not need to do so.

I will remove that from the patch, but before spending time I would like to have acknowledgement feom JV

eolivelli · 2018-06-13T09:51:24Z

@sijie @jvrao FYI I have added a commit to drop 'error out pending adds" in case of failure in force operation.

So we are not erroring out pending adds, they will fail naturally in case of bookie failure.
a force() which fails simply does not advance LastAddConfirmed.

Hopefully @sijie and @jiazhai will be still okay with the patch

jvrao · 2018-06-14T19:07:13Z

@eolivelli thanks a lot for pushing this change forward. Great work.!!

eolivelli · 2018-06-15T06:34:33Z

@jvrao @sijie @jiazhai
Thank you !

I have rebased to latest master, I will wait for CI to pass and then merge.
This was the last important part of BP-14 !
I will follow up with docs

I think existing BC tests are enough to guarantee that we did not break compatibility.

Now that BP-14 has got its final form I will start tests and benchmarks on downstream applications (once this change reaches Apache Snapshots Repository)

eolivelli · 2018-06-15T08:31:43Z