BP-22: Separate closing ledgers from opening ledgers

[ivankelly]

Proposes removing OpenOpBuilder#withRecovery and replacing it with an
explicit #seal call.

[eolivelli]

typo: compatability

[eolivelli]

why not
CompletableFuture<Void> seal(long ledgerId)
or
CompletableFuture<Void> newSealOp().withLedgerId(ledgerId).execute();

Maybe we we could return the LAC instead of Void.

The user need to re-open the ledger but it is clearer

[ivankelly]

The user needs more than the ledgerId to seal the ledger. They need digest type and password at least. In the case of log0 they'll also need the ensemble. It's more foolproof to pull this from a ReadHandle (via getLedgerMetadata) than to require the user to pass each one in.

There's no need to reopen. It should be possible to reread the LAC and updated metadata with the existing handle.

[eolivelli]

ok.
As WriteHandle extends ReadHandle this new API can be used with a an existing WriteHandle, we should forbid this.

counter proposal:

1. drop OpenBuilder#withRecovery(true)
2. add OpenBuilder#sealing()

[ivankelly]

As WriteHandle extends ReadHandle this new API can be used with a an existing WriteHandle, we should forbid this.

Why? We'd be performing a seal operation on a Ledger in either case.

counter proposal:
drop OpenBuilder#withRecovery(true)
add OpenBuilder#sealing()

I had considered something like this. It's better than withRecovery(), but you still need to reopen the ledger, sealing is still hidden inside a builder, and it makes open operation with possible side effects, which I'd like to avoid (similar to not putting #forceSealed on ReadHandle).

[sijie]

I think what Enrico proposed with a separate seal call makes more sense to me. In some cases (e.g. admin cases), you can use this seal call to seal a ledger.

changing withRecovery(true) to #sealing() works for me as well. but I would prefer not removing withRecovery(true) or #sealing() from the open builder, we should have this flag in the builder for opening ledger to seal/fence.

[ivankelly]

I think what Enrico proposed with a separate seal call makes more sense to me. In some cases (e.g. admin cases), you can use this seal call to seal a ledger.

What do you mean by a separate seal call?

changing withRecovery(true) to #sealing() works for me as well. but I would prefer not removing withRecovery(true) or #sealing() from the open builder, we should have this flag in the builder for opening ledger to seal/fence.

Why?

In any case, if we do leave it in the builder, we should call it #forceSealed or #forceClosed, to inform the user that the option has heavy side effects. In fact, I think any method on BookKeeper should be called the same.

[sijie]

@ivankelly

I mean what Enrico proposed CompletableFuture<Void> seal(long ledgerId) is making more sense to me than CompletableFuture<Void> seal(ReadHandle rh). seal call is separated from open from this point of view.

I am fine with calling it forceSealed or forceClosed in the open builder. as I said in the other comment: "I think open-to-seal is more naturally fit in open builder, as a boolean flag (withRecovery(true)), or as a OpenFlag (O_SEAL or O_FENCE or O_RECOVERY or whatever), or whatever name making sense (#sealing() as what @eolivelli proposed)".

[sijie]

The flag is called withRecovery, while the to check if recovery is needed, we call isClosed

I don't quite understand the comment here.

[ivankelly]

We alternate between recovery and closed in the api. The user then has to know that they mean the same thing, that recovery needs to run for a ledger to be considered closed.

[sijie]

I am not sure how do you plan to implement seal(ReadHandle). It seems to me it is non-trivial interface thing, you basically need to implement some logic in ReadHandle. I can foresee reusing a readhandle can have a lot of potentially problems. I am preferring the second one (which we currently have now), it has 2-3 lines more code, but it has much clean implementation: when the leadership is changed, the new leader takes over the leadership, open the ledger to fence the writer, and read entries. It deals with ledger metadata changes much better - in other words, I can see the second approach is better for distributedlog implementation.

frankly speaking, I don't see the approach proposed here is better than existing approach. there is unnecessary code changes introduced and can be error prone. bookkeeper should just provide primitives, rather than dealing with state transition (e.g. follower to leader transition).

[ivankelly]

I am not sure how do you plan to implement seal(ReadHandle).

In the simplest case it would be trivial.

CompletableFuture<Void> seal(ReadHandle lh) {
    CompletableFuture<Void> future = new CompletableFuture<>();
    this.asyncOpenLedger(lh.getId(), 
        lh.getLedgerMetadata().getPassword(),
        lh.getLedgerMetadata().getDigestType(),
        (rc, lh, ctx) -> {
                if (rc == BKException.Code.OK) {
                    future.complete(null);
                } else {
                    future.completeExceptionally(BKException.create(rc));
                }
         }, null);
    return future;
}

The passed in ReadHandle isn't modified at all, it's just used to pull the details of the ledger. I can imagine we would factor out the recovery operation in the future, but right now, all these are still implemented with LedgerHandle.

It seems to me it is non-trivial interface thing, you basically need to implement some logic in ReadHandle.

If anything it needs stuff from WriteHandle. Right now we have this really weird class hierarchy. WriteHandle subclasses ReadHandle, but ReadOnlyLedgerHandle subclasses LedgerHandle. Sealing is an writing operation, so it more naturally fits with WriteHandle, but I don't think it even belongs there. It's a one off operation for the ledger, so it belongs at the BookKeeper client.

It deals with ledger metadata changes much better

Why do you think it deals with metadata better?

bookkeeper should just provide primitives, rather than dealing with state transition (e.g. follower to leader transition).

I'm not adding anything about state transitions.

[sijie]

The passed in ReadHandle isn't modified at all, it's just used to pull the details of the ledger.

you can't simply saying you are not touching ReadHandle. You indeed need to touch ReadHandle, and mutate its state. The propose seal(ReadHandle) is effectively same as ReadHandle#seal(). I don't like this design. ReadHandle should be non invasive.

If anything it needs stuff from WriteHandle. Right now we have this really weird class hierarchy.

It is not. WriteHandle is effectively a handle that provides write and read, which is use cases like Pulsar needs. ReadHandle is the readonly handle that does non read operations only, which is use cases like DistributedLog, storage interfaces needs. It is as good as what it provides clean interfaces for use cases.

Again, I think this proposal is just another form of #774 . I think open-to-seal is more naturally fit in open builder, as a boolean flag (withRecovery(true)), or as a OpenFlag (O_SEAL or O_FENCE or O_RECOVERY or whatever), or whatever name making sense (#sealing() as what @eolivelli proposed).

As bookkeeper, it should be providing primitives, not dealing with the state transition within a handle.

[ivankelly]

you can't simply saying you are not touching ReadHandle. You indeed need to touch ReadHandle, and mutate its state. The propose seal(ReadHandle) is effectively same as ReadHandle#seal(). I don't like this design. ReadHandle should be non invasive.

ReadHandle isn't being modified, it's just a container to pull in the detailed required to seal the ledger. If this was C++ it would be const. In fact, it shouldn't be ReadHandle at all, but Handle that is passed in, as that has everything needed in #788 .

Again, I think this proposal is just another form of #774 .

In a way, #774 was about changing the default, so that users are forced to take sealing into account. This change takes a different approach, by at least making sealing prominent in the API.

As bookkeeper, it should be providing primitives, not dealing with the state transition within a handle.

Where am I suggesting we deal with state transitions? All I'm proposing is that we provide an explicit primitive, to allow the transition to take place.

[ivankelly]

btw, i'm fine with leaving it in the builder also to allow "open-to-seal", but it has to be renamed.

[sijie]

I think what Enrico proposed with a separate seal call makes more sense to me. In some cases (e.g. admin cases), you can use this seal call to seal a ledger.

changing withRecovery(true) to #sealing() works for me as well. but I would prefer not removing withRecovery(true) or #sealing() from the open builder, we should have this flag in the builder for opening ledger to seal/fence.

[sijie]

It would be nice to be able to continue using this Handle to read to the end of the ledger.

I would not say it is "nice". when you are reusing readhandle, you need to deal with a lot of corner cases during transition. it is not "nice" than opening a brand new readhandle.

[ivankelly]

which corner cases? if using the same handle, you seal it, and then read to the end of the handle. Sealing it doesn't actually modify anything in the handle. I don't see where the corner case fits in.

[sijie]

I mean if you want to implement this seal(readhandle), you might potentially encounter unknown cases around lac, length, metadata and such. what I don't like this proposal is you are proposing mutating ReadHandle. people should just simply use open to get a readhandle to tailing, and open-to-seal a ledger to get a readhandle to read. The readhandle returned by open calls should be remain as it is, there should be no state transition apply to a read handle. those state transition should happen at applicaiton level, what bookkeeper should provide is just primitives.

in short, IMO, we should

• not add any method like seal or forceClose to a ReadHandle
• not add any method like seal or forceClose to BookKeeper to change a ReadHandle

I mean CompletableFuture<Void> seal(ReadHandle handle) is effectively same as ReadHandle#seal().

[jvrao]

Why are we saying 'tailing is the primary usecase'? We need to have an API OpenForRead and OpenForReadRecovery. If we want to be deliberate, lets come up with explicit API Names.
OpenforReadRecovery() OpenForReadNoRecovery() / OpenForReadTailing() This way we keep the existing functionality and have non-ambigious API names. Basically there is no OpenForReadOnly()

[ivankelly]

I mean CompletableFuture seal(ReadHandle handle) is effectively same as ReadHandle#seal().

No, it's not. In CompletableFuture seal(ReadHandle handle), the full handle is passed just so that you don't have to do something like, seal(handle.getId(), handle.getPassword(), handle.getDigestType()), nothing in the ReadHandle itself is modified. The ReadHandle will update it's sealed status when it reads from ZK (if it's watching metadata, which in tailing it should be), but this is the same as the ReadHandles on any other node which didn't do the sealing.

I'm thinking a Op + Builder makes more sense now, so we would do something like bk.forceSealedOp().forLedger(handle).execute()
Then we could also have
bk.forceSealedOp().withLedgerId(handle.getId()).withPassword(...).withDigestType(...).execute()
This also lends itself better to log0 usecases (I want the API impact of log0 to be as small as possible), as we could have bk.forceSealedOp().forLedger(handle).creatingNextLedger().execute()

@jvrao This is the impression I got from the #774, that streaming should be the default, hence tailing being primary target usecase.

[sijie]

• not we are not saying streaming is the primary purpose. the points in Open should run recovery by default #774 are open by default should be non invasive.

• I think we shouldn't have a api that would touch ReadHandle. bk.forceSealedOp.withLedgerId(..) works for me.

[ivankelly]

not we are not saying streaming is the primary purpose. the points in #774 are open by default should be non invasive.

This givens precedence to use cases where the ledger is read while it is being written to. (i.e. streaming).

I think we shouldn't have a api that would touch ReadHandle. bk.forceSealedOp.withLedgerId(..) works for me.

As I said in another comment this morning, it doesn't need ReadHandle, Handle is sufficient. Handle is a conceptually a read-only.
The problem with withLedgerId(..) is that it isn't just withLedgerId(). It's withPassword & withDigestType() as well. So the call would look like:

bk.newForceSealedOp().withLedgerId(h.getId()).withPassword(h.getLedgerMetadata().getPassword()).withDigestType(h.getLedgerMetadata().getDigestType()).execute()

open-to-seal currently has the same problem.

bk.newOpenLedgerOp().withLedgerId(h.getId()).withPassword(h.getLedgerMetadata().getPassword()).withDigestType(h.getLedgerMetadata().getDigestType()).withRecovery(true).execute()

That's a lot of text to infer an intent from, and it's easy to miss things.
Compare with:

bk.newForceSealedOp().forLedger(h).execute()

[sijie]

yes. you might write more code. but it is much cleaner. because it enforces people to reopen to get a clean handle after seal. The biggest concern that I have is reusing LedgerHandle after seal. I would prefer : when you moving from a "tailing" readhandle to a "sealed" readhandle, close the "tailing" readhandle, sealit and reopen a branch new one. That is what I have been saying : bookkeeper should only provide primitives, should not involve any state transition. applications use primitives to do their own transition.

That's a lot of text to infer an intent from, and it's easy to miss things.

well. execute does verification. it prevents you miss things.

[ivankelly]

it enforces people to reopen to get a clean handle after seal.

No it doesn't. If the ReadHandle will be watching the metadata, and when it changes will update. Tailing has to be able to read to the end of a sealed ledger, as there may be other nodes, which are not doing the sealing which need to read to the end. The BookKeeper#newForceSealedOp could just as easily be running on a different machine or with a different BookKeeper client. It's an external event happening on the ledger which the ReadHandle points to, and the ReadHandle needs to be able to handle that.

I've no problem with having withLedgerId, withPassword, withDigest, alongside fromLedger(Handle). In fact, that's how I had been thinking of it since I started liking the idea of the builder.

well. execute does verification. it prevents you miss things.

Execution doesn't verify that you've called withRecovery correctly, or that you've called it at all, which is why I was pushing for defaulting to recovery in the first place. Large, dense blocks of text are hard to verify at a glance, and if something as important as recovery is hidden among it, people are going to make mistakes.

[sijie]

@ivankelly - There are too many back and forth here. let me try to summarize my opinion here. so it is clear rather than spreading over multiple comments.

• I am -1 on CompletableFuture<Void> seal(ReadHandle handle). To me, 1) this call makes me feel it is mutating a readhandle, which I don't feel comfortable. 2) it has similar meaning as ReadHandle#seal(), which doesn't sound correct to me. ReadHandle shouldn't any invasive methods.
• If this BP is proposing separating the seal operation from open operation, I would prefer what @eolivelli proposed. the seal operation should only deal with a ledgerId rather than a ReadHandle. So a seal operation builder with ledgerId works for me. I will give my +1 if that is the case.
• Assume we agree on a seal builder eventually, I would still prefer open builder keep withRecovery(true) there. I need this for distributedlog. I am okay with renaming it from #forceClose, #forceSeal, sealing or whatever name we agree on in the open builder.

All these are just my opinions (they can be wrong, but they are based on all the experiences that I deal with ledgers). Let's see other's opinions.

[ivankelly]

@sijie

There are too many back and forth here.
Indeed, and I've yet to see anyone else even agree that the problems I am perceiving exist.

These are issues I came across while updating the tutorial to use the new api. IMO, the tutorial was less clear in its purpose with the new api.

I'm going to stop pushing this, and #790, but also, someone else should take the task of updating the tutorial. That someone shouldn't be someone who had input to the new api definition, so that they're coming at it with fresh eyes.