This is an automated email from the ASF dual-hosted git repository.
sijie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new b07732e BP-21: New API close inconsistencies (#790)
b07732e is described below
commit b07732e29df22acabb0a7b8198cdff41c04bb27b
Author: Ivan Kelly <[email protected]>
AuthorDate: Wed Dec 6 02:13:48 2017 +0100
BP-21: New API close inconsistencies (#790)
State: Rejected
Reason: Rejected due to lack of agreement that the issues raised in the
motivation are valid.
---
site/bps/BP-21-new-api-close-inconsistencies.md | 65 +++++++++++++++++++++++++
site/community/bookkeeper_proposals.md | 2 +
2 files changed, 67 insertions(+)
diff --git a/site/bps/BP-21-new-api-close-inconsistencies.md
b/site/bps/BP-21-new-api-close-inconsistencies.md
new file mode 100644
index 0000000..4bc1ff8
--- /dev/null
+++ b/site/bps/BP-21-new-api-close-inconsistencies.md
@@ -0,0 +1,65 @@
+---
+title: "BP-21: New API close inconsistencies"
+issue: https://github.com/apache/bookkeeper/issues/789
+state: "Rejected"
+release: "4.6.0"
+---
+
+Rejected due to lack of agreement that the issues raised in the motivation are
valid.
+
+### Motivation
+
+The
[Handle](http://bookkeeper.apache.org/docs/latest/api/javadoc/org/apache/bookkeeper/client/api/Handle.html)
interface provides two methods, #asyncClose and #close (overriding
AutoCloseable).
+
+#close is implemented in both
[ReadHandle](http://bookkeeper.apache.org/docs/latest/api/javadoc/org/apache/bookkeeper/client/api/ReadHandle.html)
and
[WriteHandle](http://bookkeeper.apache.org/docs/latest/api/javadoc/org/apache/bookkeeper/client/api/WriteHandle.html).
+
+1. The implementations in ReadHandle and WriteHandle do vastly different
things. In ReadHandle, #close unregisters listeners from the ledger manager.
This is local resource cleanup, which is in line with what AutoCloseable is
designed for. In WriteHandle, #close calls #asyncClose which writes the
lastAddConfirmed to the LedgerMetadata. This violates the principle of
separation of concerns, and overloads the meaning of the term "close".
+
+2. #asyncClose is defined in Handle, but it only has any meaning in the
WriteHandle. In ReadHandle, closing only cleans up local resources, there's no
network nor disk I/O involved. The implementation directly calls the callback.
It's only in WriteHandle that asyncClose has any meaning, and here it is
completely different to in ReadHandle.
+
+3. The name #asyncClose is inconsistent with every other method on the new api
Handles (append, read, readLastAddConfirmed, etc).
+
+4. #close is part of AutoClosable, so its not unreasonable for it to be used
in a try-with-resource block. This means that a ledger closure (i.e.
distributed state mutation) could be triggered by an exception within the
block. This is nasty.
+
+Overloading the meaning of the term "close" is very problematic on its own.
Closing a WriteHandle is a very important part of the BookKeeper protocol, so
it should at least have it's own _verb_. I propose that we stop using "closing
a ledger" to describe setting the last entry of a ledger, and instead call it
"sealing a ledger".
+
+### Public Interfaces
+
+1. Remove Handle#asyncClose
+2. Add new method WriteHandle#seal.
+```
+class WriteHandle {
+ CompletableFuture<Void> seal();
+}
+```
+
+### Proposed Changes
+
+The proposed change remove asyncClose from all handles and replaces it with a
async #seal method on WriteHandle. WriteHandle will still have a #close method
for cleaning up local resources.
+
+The proposed usage would look like:
+
+```
+try (WriteHandle writer =
bk.newCreateLedgerOp().withPassword("bleh".getBytes()).execute().get()) {
+ for (int i = 0; i < 100; i++) {
+ writer.append("foobar".getBytes());
+ }
+ writer.seal().get(); // no more entries can be added
+}
+```
+
+_What if the user forgets to call #seal before closing the ledger?_
+
+The ledger is left unsealed. Readers will not read past the end of the
unsealed ledger, and they will either try to recover the ledger or wait
forever. In both cases, the consistency of the data is guaranteed as the writer
would only acknowledge writes which have hit the full ack quorum, which will
always be picked up by recovery. If the writer had made any writes that were
not acknowledged, it would have halted and not moved onto writing a new ledger.
+
+### Compatibility, Deprecation, and Migration Plan
+
+None, this only affects the new api.
+
+### Test Plan
+
+The current tests for #asyncClose will be migrated to use #seal().
+
+### Rejected Alternatives
+
+The alternative is how it is now. The movitation section describes the problem
with this.
diff --git a/site/community/bookkeeper_proposals.md
b/site/community/bookkeeper_proposals.md
index 434ec1a..adbc9fb 100644
--- a/site/community/bookkeeper_proposals.md
+++ b/site/community/bookkeeper_proposals.md
@@ -98,6 +98,7 @@ Proposal | State
[BP-16: Thin Client - Remove direct metadata storage access from
clients](https://cwiki.apache.org/confluence/display/BOOKKEEPER/BP-16%3A+Thin+Client+-+Remove+direct+metadata+storage+access+from+clients)
| Draft
[BP-18: LedgerType, Flags and
StorageHints](https://cwiki.apache.org/confluence/display/BOOKKEEPER/BP-18%3A+LedgerType%2C+Flags+and+StorageHints)
| Accepted
+
### Adopted
Proposal | Release
@@ -120,3 +121,4 @@ Proposal | Release
Proposal | Reason
:--------|:------
[BP-7 - Explicit LAC on
addEntry](https://cwiki.apache.org/confluence/display/BOOKKEEPER/BP-7+-+Explicit+LAC+on+addEntry)
| Not A Problem
+[BP-21: New API close
inconsistencies](../../bps/BP-21-new-api-close-inconsistencies) | Not A Problem
--
To stop receiving notification emails like this one, please contact
['"[email protected]" <[email protected]>'].
