Build failed in Jenkins: distributedlog-nightly-build #495

[Apache Jenkins Server]

See 


--
[...truncated 253.78 KB...]
[INFO] Downloaded: 
http://repository.apache.org/snapshots/org/apache/bookkeeper/bookkeeper-common/4.6.0-SNAPSHOT/bookkeeper-common-4.6.0-20171122.071203-60.pom
 (4.4 kB at 11 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.pom
 (4.3 kB at 330 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/com/google/errorprone/error_prone_annotations/2.1.2/error_prone_annotations-2.1.2.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/com/google/errorprone/error_prone_annotations/2.1.2/error_prone_annotations-2.1.2.pom
 (1.8 kB at 149 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/com/google/errorprone/error_prone_parent/2.1.2/error_prone_parent-2.1.2.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/com/google/errorprone/error_prone_parent/2.1.2/error_prone_parent-2.1.2.pom
 (5.1 kB at 390 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/commons-configuration/commons-configuration/1.10/commons-configuration-1.10.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/commons-configuration/commons-configuration/1.10/commons-configuration-1.10.pom
 (21 kB at 1.7 MB/s)
[INFO] Downloading: 
http://repository.apache.org/snapshots/org/apache/bookkeeper/bookkeeper-proto/4.6.0-SNAPSHOT/maven-metadata.xml
[INFO] Downloaded: 
http://repository.apache.org/snapshots/org/apache/bookkeeper/bookkeeper-proto/4.6.0-SNAPSHOT/maven-metadata.xml
 (1.0 kB at 2.4 kB/s)
[INFO] Downloading: 
http://repository.apache.org/snapshots/org/apache/bookkeeper/bookkeeper-proto/4.6.0-SNAPSHOT/bookkeeper-proto-4.6.0-20171122.071040-12.pom
[INFO] Downloaded: 
http://repository.apache.org/snapshots/org/apache/bookkeeper/bookkeeper-proto/4.6.0-SNAPSHOT/bookkeeper-proto-4.6.0-20171122.071040-12.pom
 (2.6 kB at 6.6 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/com/google/protobuf/protobuf-java/3.4.0/protobuf-java-3.4.0.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/com/google/protobuf/protobuf-java/3.4.0/protobuf-java-3.4.0.pom
 (4.7 kB at 365 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/com/google/protobuf/protobuf-parent/3.4.0/protobuf-parent-3.4.0.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/com/google/protobuf/protobuf-parent/3.4.0/protobuf-parent-3.4.0.pom
 (6.8 kB at 570 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/commons-io/commons-io/2.4/commons-io-2.4.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/commons-io/commons-io/2.4/commons-io-2.4.pom
 (10 kB at 847 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/commons/commons-parent/25/commons-parent-25.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/commons/commons-parent/25/commons-parent-25.pom
 (48 kB at 3.2 MB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/commons/commons-lang3/3.3.2/commons-lang3-3.3.2.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/commons/commons-lang3/3.3.2/commons-lang3-3.3.2.pom
 (20 kB at 1.7 MB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/commons/commons-collections4/4.1/commons-collections4-4.1.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/commons/commons-collections4/4.1/commons-collections4-4.1.pom
 (21 kB at 1.9 MB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/net/java/dev/jna/jna/3.2.7/jna-3.2.7.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/net/java/dev/jna/jna/3.2.7/jna-3.2.7.pom 
(2.1 kB at 189 kB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/io/netty/netty-all/4.1.12.Final/netty-all-4.1.12.Final.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/io/netty/netty-all/4.1.12.Final/netty-all-4.1.12.Final.pom
 (25 kB at 2.1 MB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/io/netty/netty-tcnative-boringssl-static/2.0.3.Final/netty-tcnative-boringssl-static-2.0.3.Final.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/io/netty/netty-tcnative-boringssl-static/2.0.3.Final/netty-tcnative-boringssl-static-2.0.3.Final.pom
 (22 kB at 1.7 MB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/io/netty/netty-tcnative-parent/2.0.3.Final/netty-tcnative-parent-2.0.3.Final.pom
[INFO] Downloaded: 
https://repo.maven.apache.org/maven2/io/netty/netty-tcnative-parent/2.0.3.Final/netty-tcnative-parent-2.0.3.Final.pom
 (20 kB at 1.7 MB/s)
[INFO] Downloading: 
https://repo.maven.apache.org/maven2/io/netty/netty-parent/4.0.18.Final/netty-parent-4.0.18.Final.pom
[INFO] Downloaded: 

[GitHub] merlimat commented on issue #754: Issue #753: Allow option to disable data sync on journal

[GitBox]

merlimat commented on issue #754: Issue #753: Allow option to disable data sync 
on journal
URL: https://github.com/apache/bookkeeper/pull/754#issuecomment-346540585
 
 
   @eolivelli Updated the docs for the flag. PTAL


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai closed pull request #767: Issue 766: Bump master to 4.7.0-SNAPSHOT

[GitBox]

jiazhai closed pull request #767: Issue 766: Bump master to 4.7.0-SNAPSHOT
URL: https://github.com/apache/bookkeeper/pull/767
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/bookkeeper-benchmark/pom.xml b/bookkeeper-benchmark/pom.xml
index 01c7bbba2..d52392351 100644
--- a/bookkeeper-benchmark/pom.xml
+++ b/bookkeeper-benchmark/pom.xml
@@ -21,7 +21,7 @@
   
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
   
   org.apache.bookkeeper
   bookkeeper-benchmark
diff --git a/bookkeeper-common/pom.xml b/bookkeeper-common/pom.xml
index 9051b59a6..b1c624e6b 100644
--- a/bookkeeper-common/pom.xml
+++ b/bookkeeper-common/pom.xml
@@ -21,7 +21,7 @@
   
 org.apache.bookkeeper
 bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
   
   bookkeeper-common
   Apache BookKeeper :: Common
diff --git a/bookkeeper-dist/all/pom.xml b/bookkeeper-dist/all/pom.xml
index ac4be99ec..ddb8aebb9 100644
--- a/bookkeeper-dist/all/pom.xml
+++ b/bookkeeper-dist/all/pom.xml
@@ -24,7 +24,7 @@
   
 bookkeeper-dist
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 ..
   
 
diff --git a/bookkeeper-dist/pom.xml b/bookkeeper-dist/pom.xml
index f93be132e..d487568b0 100644
--- a/bookkeeper-dist/pom.xml
+++ b/bookkeeper-dist/pom.xml
@@ -19,7 +19,7 @@
   
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
   
   4.0.0
   bookkeeper-dist
diff --git a/bookkeeper-dist/server/pom.xml b/bookkeeper-dist/server/pom.xml
index e46e99491..10bd5b7b4 100644
--- a/bookkeeper-dist/server/pom.xml
+++ b/bookkeeper-dist/server/pom.xml
@@ -24,7 +24,7 @@
   
 bookkeeper-dist
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 ..
   
 
diff --git a/bookkeeper-http/http-server/pom.xml 
b/bookkeeper-http/http-server/pom.xml
index 194bd4a35..e9b1f3993 100644
--- a/bookkeeper-http/http-server/pom.xml
+++ b/bookkeeper-http/http-server/pom.xml
@@ -21,7 +21,7 @@
 
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 ../..
 
 4.0.0
diff --git a/bookkeeper-http/pom.xml b/bookkeeper-http/pom.xml
index 5c05d88e6..815aa99f7 100644
--- a/bookkeeper-http/pom.xml
+++ b/bookkeeper-http/pom.xml
@@ -21,7 +21,7 @@
 
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 
 4.0.0
 org.apache.bookkeeper.http
diff --git a/bookkeeper-http/twitter-http-server/pom.xml 
b/bookkeeper-http/twitter-http-server/pom.xml
index db874b182..81f4fee19 100644
--- a/bookkeeper-http/twitter-http-server/pom.xml
+++ b/bookkeeper-http/twitter-http-server/pom.xml
@@ -21,7 +21,7 @@
 
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 ../..
 
 4.0.0
diff --git a/bookkeeper-http/vertx-http-server/pom.xml 
b/bookkeeper-http/vertx-http-server/pom.xml
index ea5b26d57..bf6c6721b 100644
--- a/bookkeeper-http/vertx-http-server/pom.xml
+++ b/bookkeeper-http/vertx-http-server/pom.xml
@@ -21,7 +21,7 @@
 
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 ../..
 
 4.0.0
diff --git a/bookkeeper-proto/pom.xml b/bookkeeper-proto/pom.xml
index 02bf4ce22..cc9169da7 100644
--- a/bookkeeper-proto/pom.xml
+++ b/bookkeeper-proto/pom.xml
@@ -20,7 +20,7 @@
   
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
   
   bookkeeper-proto
   Apache BookKeeper :: Protocols
diff --git a/bookkeeper-server/pom.xml b/bookkeeper-server/pom.xml
index bf7f99cf8..28d90ba0e 100644
--- a/bookkeeper-server/pom.xml
+++ b/bookkeeper-server/pom.xml
@@ -20,7 +20,7 @@
   
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
   
   bookkeeper-server
   Apache BookKeeper :: Server
diff --git a/bookkeeper-stats-providers/codahale-metrics-provider/pom.xml 
b/bookkeeper-stats-providers/codahale-metrics-provider/pom.xml
index 9b76ca319..5e80d58e1 100644
--- a/bookkeeper-stats-providers/codahale-metrics-provider/pom.xml
+++ b/bookkeeper-stats-providers/codahale-metrics-provider/pom.xml
@@ -20,7 +20,7 @@
   
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
 ../..
   
   org.apache.bookkeeper.stats
diff --git a/bookkeeper-stats-providers/pom.xml 
b/bookkeeper-stats-providers/pom.xml
index a493f64d4..31ad9f180 100644
--- a/bookkeeper-stats-providers/pom.xml
+++ b/bookkeeper-stats-providers/pom.xml
@@ -19,7 +19,7 @@
   
 bookkeeper
 org.apache.bookkeeper
-4.6.0-SNAPSHOT
+4.7.0-SNAPSHOT
   
   4.0.0
   bookkeeper-stats-providers
diff --git a/bookkeeper-stats-providers/prometheus-metrics-provider/pom.xml 

[GitHub] jiazhai closed issue #766: Bump master to 4.7.0-SNAPSHOT

[GitBox]

jiazhai closed issue #766: Bump master to 4.7.0-SNAPSHOT
URL: https://github.com/apache/bookkeeper/issues/766
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on issue #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

eolivelli commented on issue #727: Issue 693: add interface and implementation 
of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#issuecomment-346538747
 
 
   I am taking last look


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new pull request #768: Issue 763: Javadoc layout should be reflected to new modules

[GitBox]

sijie opened a new pull request #768: Issue 763: Javadoc layout should be 
reflected to new modules
URL: https://github.com/apache/bookkeeper/pull/768
 
 
   Descriptions of the changes in this PR:
   
   - add `org.apache.bookkeeper.client.api` in `BookKeeper Client (New Fluent 
API - Experimental)`
   - move prometheus package to `BookKeeper Stats Providers`
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152717258
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   Thanks @ivankelly and @sijie for your comments. 
   I will remove it now, and think of it later when it is wanted.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152717258
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   Thanks @ivankelly and @sijie for your comments. 
   I will remove it now, and think of it later to see if this is indeed needed.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #766: Bump master to 4.7.0-SNAPSHOT

[GitBox]

sijie commented on issue #766: Bump master to 4.7.0-SNAPSHOT
URL: https://github.com/apache/bookkeeper/issues/766#issuecomment-346521705
 
 
   /cc @jiazhai for release manager to be aware of this.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #764: Issue-553: Documentation for new fluent API

[GitBox]

sijie commented on issue #764: Issue-553: Documentation for new fluent API
URL: https://github.com/apache/bookkeeper/pull/764#issuecomment-346521719
 
 
   /cc @lucperkins 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #766: Bump master to 4.7.0-SNAPSHOT

[GitBox]

sijie opened a new issue #766: Bump master to 4.7.0-SNAPSHOT
URL: https://github.com/apache/bookkeeper/issues/766
 
 
   *PROBLEM*
   
   The master was not bumped to `4.7.0` when branch-4.6 was created.
   
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #552: Update BookKeeper Tutorial to the new API

[GitBox]

sijie commented on issue #552: Update BookKeeper Tutorial to the new API
URL: https://github.com/apache/bookkeeper/issues/552#issuecomment-346521445
 
 
   @ivankelly just check in here - are you picking this up? or shall I take it?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #239: Use new bookkeeper fluent API

[GitBox]

sijie opened a new issue #239: Use new bookkeeper fluent API
URL: https://github.com/apache/distributedlog/issues/239
 
 
   
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   A new bookkeeper fluent API is introduced in apache/bookkeeper#506. 
DistributedLog should integrate with the new fluent API to leverage 
`CompletableFuture`.
   
   This task can also be used as a sanity check for verifying if the new 
bookkeeper API can take replace of the existing API.
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   This should be a *blocker* for `release/0.6.0`. 
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #765: Add `isClosed()` to WriteHandle and ReadHandle

[GitBox]

sijie opened a new issue #765: Add `isClosed()` to WriteHandle and ReadHandle
URL: https://github.com/apache/bookkeeper/issues/765
 
 
   
   1. Please describe the feature you are requesting.
   
   The new API is missing `isClosed()` on handles. It is hard to learn whether 
a ledger is closed or not for tailing-read applications.
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   *blocker*
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #728: Issue-553 Documentation for new API (WIP)

[GitBox]

sijie commented on issue #728: Issue-553 Documentation for new API (WIP)
URL: https://github.com/apache/bookkeeper/pull/728#issuecomment-346520247
 
 
   @eolivelli I sent the pull request #764 . I will close this one since there 
is no activity on this one anymore.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie closed pull request #728: Issue-553 Documentation for new API (WIP)

[GitBox]

sijie closed pull request #728: Issue-553 Documentation for new API (WIP)
URL: https://github.com/apache/bookkeeper/pull/728
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/site/docs/4.6.0/api/ledger-api.md 
b/site/docs/4.6.0/api/ledger-api.md
index 4e1070d71..4e1d45af3 100644
--- a/site/docs/4.6.0/api/ledger-api.md
+++ b/site/docs/4.6.0/api/ledger-api.md
@@ -264,6 +264,88 @@ Result: 2
 # etc
 ```
 
+## New API
+
+Since 4.6 BookKeeper provides a new client API which leverages Java 
CompletableFuture facility.
+
+We do not have a generic LedgerHandle class but more specific interfaces 
WriteHandle and ReadHandle.
+
+> All the new API is in the org.apache.bookkeeper.client.api, you should only 
use classes in this package.
+
+*Beware* that this API in 4.6 is still experimental and could be subject to 
changes in next major releases
+
+In order to create the BookKeeper client use the BookKeeper.Builder API
+
+  ```java
+import org.apache.bookkeeper.client.api.*;
+
+  BookKeeper bkClient = BookKeeper.forConfig(conf).build();
+  ```
+
+In order to create a new Ledger and write to it use the Create API
+
+   ```java
+import org.apache.bookkeeper.client.api.*;
+
+ WriteHandle writer = bkClient.newCreateLedgerOp()
+.withAckQuorumSize(ackQuorumSize)
+.withEnsembleSize(ensembleSize)
+.withWriteQuorumSize(writeQuorumSize)
+.withCustomMetadata(customMetadata)
+.withPassword(password)
+.execute()
+.get();
+ 
+ int numberOfEntries = 100;
+
+ // Add entries to the ledger, then close it
+ for (int i = 0; i < numberOfEntries; i++){
+ByteBuffer entry = ByteBuffer.allocate(4);
+   entry.putInt(i);
+   entry.position(0);
+   long entryId = lh.append().get();
+ }
+ writer.close();
+  ```
+
+In order to read from a ledger you have to create a ReadHandle
+
+
+  ReadHandle reader = bkClient.newOpenLedgerOp()
+.withPassword(ledgerMetadata.getPassword())
+.withDigestType(DigestType.CRC32)
+.withLedgerId(ledgerId)
+.withRecovery(true)
+.execute()
+.get();
+  long lastAddConfirmed = reader.getLastAddConfirmed();
+  Iterable entries = reader.read(0, lastAddConfirmed).get();
+  reader.close();
+
+ Most of the methods of the new API return CompletableFutures and you have to 
call get()
+ to block and wait for a result or an error.
+ You can use org.apache.bookkeeper.common.concurrent.FutureUtils.result utility
+ which will handle ExecutionException for you.
+
+   ```java
+import static org.apache.bookkeeper.common.concurrent.FutureUtils.result;
+
+try (ReadHandle reader = result(bkClient.newOpenLedgerOp()
+.withPassword(ledgerMetadata.getPassword())
+.withDigestType(DigestType.CRC32)
+.withLedgerId(ledgerId)
+.withRecovery(true)
+.execute()));{
+  long lastAddConfirmed = reader.getLastAddConfirmed();
+  Iterable entries = result(reader.read(0, lastAddConfirmed));
+
+} catch (BKException error) {
+  .
+}
+   ```
+
+```
+
 ## Example application
 
 This tutorial walks you through building an example application that uses 
BookKeeper as the replicated log. The application uses the [BookKeeper Java 
client](../java-client) to interact with BookKeeper.


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new pull request #764: Issue-553: Documentation for new fluent API

[GitBox]

sijie opened a new pull request #764: Issue-553: Documentation for new fluent 
API
URL: https://github.com/apache/bookkeeper/pull/764
 
 
   Descriptions of the changes in this PR:
   
   - Add documentation for the new fluent API: create/open/delete, 
append/write/read, createadv


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on issue #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on issue #755: Issue 750: support ByteBuf, ByteBuffer, 
byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#issuecomment-346503785
 
 
   +1
   
   On Wed 22 Nov 2017, 23:48 Sijie Guo  wrote:
   
   > *@sijie* commented on this pull request.
   > --
   >
   > In
   > 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
   > :
   >
   > > @@ -41,7 +41,8 @@
   >  /**
   >   * Add entry asynchronously to an open ledger.
   >   *
   > - * @param data array of bytes to be written
   > + * @param data a bytebuf to be written. The bytebuf's reference count 
will be decremented by 1 after the
   >
   > okay I will change it to "when the returned completablefuture is completed"
   >
   > ?
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > , or 
mute
   > the thread
   > 

   > .
   >
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

sijie commented on a change in pull request #755: Issue 750: support ByteBuf, 
ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152702805
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
 ##
 @@ -41,7 +41,8 @@
 /**
  * Add entry asynchronously to an open ledger.
  *
- * @param data array of bytes to be written
+ * @param data a bytebuf to be written. The bytebuf's reference count will 
be decremented by 1 after the
 
 Review comment:
   okay I will change it to "when the returned completablefuture is completed"


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #586: Provide an DCOS Universe package for bookkeeper

[GitBox]

sijie commented on issue #586: Provide an DCOS Universe package for bookkeeper
URL: https://github.com/apache/bookkeeper/issues/586#issuecomment-346501344
 
 
   moved this to `4.7.0`. we need to make sure dcos is updated with latest 
bookkeeper release and apache/bookkeeper images.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152699968
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
 ##
 @@ -41,7 +41,8 @@
 /**
  * Add entry asynchronously to an open ledger.
  *
- * @param data array of bytes to be written
+ * @param data a bytebuf to be written. The bytebuf's reference count will 
be decremented by 1 after the
 
 Review comment:
   In the normal case, the bytebuf will have a refcount of 1. It will be passed 
to append, and then the caller can forget about the bytebuf. In this case, the 
caller is passing ownership of that 1 refcount to the callee.
   If the caller wants to keep a reference, it increments the refcount again. 
In this case, the callee will still own one refcount, and the caller will own 
another.
   
   Not too fussed about this in any case. What's there is fine. Though the 
"completable future is returned" bit is unclear. Is it when the completable 
future completes, or when the append method returns?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie closed issue #749: Update release date for 4.5.1

[GitBox]

sijie closed issue #749: Update release date for 4.5.1
URL: https://github.com/apache/bookkeeper/issues/749
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #749: Update release date for 4.5.1

[GitBox]

sijie commented on issue #749: Update release date for 4.5.1
URL: https://github.com/apache/bookkeeper/issues/749#issuecomment-346496426
 
 
   This is merged by #758 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #763: Javadoc layout should be reflected to new modules

[GitBox]

sijie commented on issue #763: Javadoc layout should be reflected to new modules
URL: https://github.com/apache/bookkeeper/issues/763#issuecomment-346494103
 
 
   @jiazhai - we need to do this for 4.6.0 release. I marked it as a blocker.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #763: Javadoc layout should be reflected to new modules

[GitBox]

sijie opened a new issue #763: Javadoc layout should be reflected to new modules
URL: https://github.com/apache/bookkeeper/issues/763
 
 
   
   
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   New modules (e.g. bookkeeper-proto) are introduced in 4.6.0 and new API is 
introduced. But the javadoc is not updated with the new modules. 
https://bookkeeper.apache.org/docs/latest/api/javadoc/
   
   We need to update the javadoc generation to make it aligned with new 
modules. The proposed changes of a new javadoc layout:
   
   - BookKeeper Client
 - org.apache.bookkeeper.client
 - org.apache.bookkeeper.conf
 - org.apache.bookkeeper.common.annotation
 - org.apache.bookkeeper.feature
   
   - BookKeeper New Client API
 - org.apache.bookkeeper.client.api
   
   - BookKeeper Stats API
 - org.apache.bookkeeper.stats
   
   - BookKeeper Stats Providers
 - org.apache.bookkeeper.stats.twitter.finagle
 - org.apache.bookkeeper.stats.twitter.ostrich
 - org.apache.bookkeeper.stats.twitter.science
 - org.apache.bookkeeper.stats.prometheus
   
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   *blocker* for `release/4.6.0`
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #762: Codahale and Prometheus stats providers should be in a different package name

[GitBox]

sijie opened a new issue #762: Codahale and Prometheus stats providers should 
be in a different package name
URL: https://github.com/apache/bookkeeper/issues/762
 
 
   
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   Codahale and Prometheus stats providers are sharing same package name with 
the stats library `org.apache.bookkeeper.stats`. We should consider moving them 
to their own package name.
   
   Codahale => `org.apache.bookkeeper.stats.codahale`
   Prometheus => `org.apache.bookkeeper.stats.prometheus`
   
   For BC concerns, we need to do this in two phases:
   
   - in 4.7, we copy the files but still have the old files for BC. but mark 
the old classes as deprecated.
   - in 4.8, we remove the old files.
   
   because the class names are used for reflection.
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   *should-have*
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #760: BP-20: Github workflow for bookkeeper proposals

[GitBox]

sijie commented on a change in pull request #760: BP-20: Github workflow for 
bookkeeper proposals
URL: https://github.com/apache/bookkeeper/pull/760#discussion_r152692268
 
 

 ##
 File path: site/bps/BP-template.md
 ##
 @@ -0,0 +1,42 @@
+---
+title: "BP-XYZ: capation of bookkeeper proposal"
+issue: https://github.com/apache/bookkeeper/
+state: "one of ['Under Discussion', 'Accepted', 'Adopted', 'Rejected']"
+release: "x.y.z"
+---
+
+### Motivation
+
+_Describe the problems you are trying to solve_
+
+### Public Interfaces
+
+_Briefly list any new interfaces that will be introduced as part of this 
proposal or any existing interfaces that will be removed or changed. The 
purpose of this section is to concisely call out the public contract that will 
come along with this feature._
+
+A public interface is any change to the following:
+
+- Data format, Metadata format
+- The wire protocol and api behavior
+- Any class in the public packages
+- Monitoring
+- Command line tools and arguments
+- Anything else that will likely break existing users in some way when they 
upgrade
+
+### Proposed Changes
+
+_Describe the new thing you want to do in appropriate detail. This may be 
fairly extensive and have large subsections of its own. Or it may be a few 
sentences. Use judgement based on the scope of the change._
+
+### Compatibility, Deprecation, and Migration Plan
+
+- What impact (if any) will there be on existing users? 
+- If we are changing behavior how will we phase out the older behavior? 
+- If we need special migration tools, describe them here.
+- When will we remove the existing behavior?
+
+### Test Plan
+
+_Describe in few sentences how the BP will be tested. We are mostly interested 
in system tests (since unit-tests are specific to implementation details). How 
will we know that the implementation works as expected? How will we know 
nothing broke?_
+
+### Rejected Alternatives
 
 Review comment:
   "Upgrade instructions" falls into "Compatibility, Deprecation, and Migration 
Plan".
   
   "New xyz" falls into "Public Interfaces"


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #760: BP-20: Github workflow for bookkeeper proposals

[GitBox]

sijie commented on a change in pull request #760: BP-20: Github workflow for 
bookkeeper proposals
URL: https://github.com/apache/bookkeeper/pull/760#discussion_r152692123
 
 

 ##
 File path: site/bps/BP-template.md
 ##
 @@ -0,0 +1,42 @@
+---
+title: "BP-XYZ: capation of bookkeeper proposal"
+issue: https://github.com/apache/bookkeeper/
+state: "one of ['Under Discussion', 'Accepted', 'Adopted', 'Rejected']"
 
 Review comment:
   yes 'Adopted' means it is done, implementation is merged and documentation 
is available. It can be announced for a given release. BP is intended for 
"design doc", not for task tracking. The task tracking should be just done in 
github issues.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] itsuugo commented on issue #328: Docker image: add a Docker Compose example file

[GitBox]

itsuugo commented on issue #328: Docker image: add a Docker Compose example file
URL: https://github.com/apache/bookkeeper/issues/328#issuecomment-346483469
 
 
   I guess that I found the problem:
   
   ```
   bookkeeper_1  | BK_CLUSTER_ROOT_PATH is
   bookkeeper_1  | BK_bookiePort bookie service port is 3181
   bookkeeper_1  | BK_zkServers is zookeeper:2181
   bookkeeper_1  | BK_DATA_DIR is /data/bookkeeper
   bookkeeper_1  | BK_CLUSTER_ROOT_PATH is
   ```
   
   BK_CLUSTER_ROOT_PATH is empty and it fails to create it as you can see in 
the log of the container
   
   ```
   bookkeeper_1  | 2017-11-22 21:24:36,524 - INFO  - 
[main:ClientCnxnSocket@236] - jute.maxbuffer value is 4194304 Bytes
   bookkeeper_1  | create [-s] [-e] [-c] [-t ttl] path [data] [acl]
   ```
   
   However, the 
[README](https://github.com/apache/bookkeeper/blob/master/docker/README.md) 
says it can be empty ? 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] itsuugo commented on issue #328: Docker image: add a Docker Compose example file

[GitBox]

itsuugo commented on issue #328: Docker image: add a Docker Compose example file
URL: https://github.com/apache/bookkeeper/issues/328#issuecomment-346483469
 
 
   I guess that I found the problem:
   
   ```
   bookkeeper_1  | BK_CLUSTER_ROOT_PATH is
   bookkeeper_1  | BK_bookiePort bookie service port is 3181
   bookkeeper_1  | BK_zkServers is zookeeper:2181
   bookkeeper_1  | BK_DATA_DIR is /data/bookkeeper
   bookkeeper_1  | BK_CLUSTER_ROOT_PATH is
   ```
   
   BK_CLUSTER_ROOT_PATH is empty and it fails to create it as you can see in 
the log of the container
   
   ```
   bookkeeper_1  | 2017-11-22 21:24:36,524 - INFO  - 
[main:ClientCnxnSocket@236] - jute.maxbuffer value is 4194304 Bytes
   bookkeeper_1  | create [-s] [-e] [-c] [-t ttl] path [data] [acl]
   ```
   
   However, the 
(README)[https://github.com/apache/bookkeeper/blob/master/docker/README.md) 
says it can be empty ? 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] itsuugo commented on issue #328: Docker image: add a Docker Compose example file

[GitBox]

itsuugo commented on issue #328: Docker image: add a Docker Compose example file
URL: https://github.com/apache/bookkeeper/issues/328#issuecomment-346483469
 
 
   I found the problem:
   
   ```
   bookkeeper_1  | BK_CLUSTER_ROOT_PATH is
   bookkeeper_1  | BK_bookiePort bookie service port is 3181
   bookkeeper_1  | BK_zkServers is zookeeper:2181
   bookkeeper_1  | BK_DATA_DIR is /data/bookkeeper
   bookkeeper_1  | BK_CLUSTER_ROOT_PATH is
   ```
   
   BK_CLUSTER_ROOT_PATH is empty and it fails to create it as you can see in 
the log of the container
   
   ```
   bookkeeper_1  | 2017-11-22 21:24:36,524 - INFO  - 
[main:ClientCnxnSocket@236] - jute.maxbuffer value is 4194304 Bytes
   bookkeeper_1  | create [-s] [-e] [-c] [-t ttl] path [data] [acl]
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152675436
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   > Anyway, let's move forward without retainedIterator. It is not a blocker 
for 4.6.0. We can circle back in future if it is needed.
   
   Let's leave it out for now then. I think with APIs it's better to leave it 
out if in doubt. It's always easier to add calls to an API than remove them.
   
   Regarding retain vs retained vs retaining, in the case of ByteBuf, it is the 
bytebuf itself that has the reference count, so it makes sense to "retain" a 
bytebuf and have a "retained" bytebuf. However, in the case of an iterator, 
it's not the iterator that is being retained. Rather the iterator is just a 
container for objects that are retained, so in some sense the iterator is 
retaining the objects, though that's not perfect either.
   
   Another thing I thought about with the retainedIterator is that error 
handling becomes awkward. If while iterating an error occurs, you still need to 
iterate to the end to free all refcounts, which isn't immediately obvious from 
the api.
   
   In summary, there seems to be a good few sharp corners on this call, so best 
to leave it out for now.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #760: BP-20: Github workflow for bookkeeper proposals

[GitBox]

eolivelli commented on a change in pull request #760: BP-20: Github workflow 
for bookkeeper proposals
URL: https://github.com/apache/bookkeeper/pull/760#discussion_r152671762
 
 

 ##
 File path: site/bps/BP-template.md
 ##
 @@ -0,0 +1,42 @@
+---
+title: "BP-XYZ: capation of bookkeeper proposal"
+issue: https://github.com/apache/bookkeeper/
+state: "one of ['Under Discussion', 'Accepted', 'Adopted', 'Rejected']"
 
 Review comment:
   Adopted is 'merged fully to master'? Or shall we have something like' 
Development in progress', 'partially implemented'...
   Users will see many things now , so it must be clear that not all the 
proposed stuff is on master or released
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #760: BP-20: Github workflow for bookkeeper proposals

[GitBox]

eolivelli commented on a change in pull request #760: BP-20: Github workflow 
for bookkeeper proposals
URL: https://github.com/apache/bookkeeper/pull/760#discussion_r152672076
 
 

 ##
 File path: site/bps/BP-template.md
 ##
 @@ -0,0 +1,42 @@
+---
+title: "BP-XYZ: capation of bookkeeper proposal"
+issue: https://github.com/apache/bookkeeper/
+state: "one of ['Under Discussion', 'Accepted', 'Adopted', 'Rejected']"
+release: "x.y.z"
+---
+
+### Motivation
+
+_Describe the problems you are trying to solve_
+
+### Public Interfaces
+
+_Briefly list any new interfaces that will be introduced as part of this 
proposal or any existing interfaces that will be removed or changed. The 
purpose of this section is to concisely call out the public contract that will 
come along with this feature._
+
+A public interface is any change to the following:
+
+- Data format, Metadata format
+- The wire protocol and api behavior
+- Any class in the public packages
+- Monitoring
+- Command line tools and arguments
+- Anything else that will likely break existing users in some way when they 
upgrade
+
+### Proposed Changes
+
+_Describe the new thing you want to do in appropriate detail. This may be 
fairly extensive and have large subsections of its own. Or it may be a few 
sentences. Use judgement based on the scope of the change._
+
+### Compatibility, Deprecation, and Migration Plan
+
+- What impact (if any) will there be on existing users? 
+- If we are changing behavior how will we phase out the older behavior? 
+- If we need special migration tools, describe them here.
+- When will we remove the existing behavior?
+
+### Test Plan
+
+_Describe in few sentences how the BP will be tested. We are mostly interested 
in system tests (since unit-tests are specific to implementation details). How 
will we know that the implementation works as expected? How will we know 
nothing broke?_
+
+### Rejected Alternatives
 
 Review comment:
   Add a section like 'documentation needed', like:
Upgrade instructions
New client API
New config options
   New protocol
New libraries


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #761: BP-20: Github workflow for bookkeeper proposals

[GitBox]

sijie opened a new issue #761: BP-20: Github workflow for bookkeeper proposals
URL: https://github.com/apache/bookkeeper/issues/761
 
 
   **BP**
   
   This is the master ticket for tracking BP-20 :
   
   Implementing a github workflow for bookkeeper proposals, so all the 
bookkeeper proposals can be reviewed as code changes, and they can be 
maintained in the same way as how we maintain documentation.
   
   Proposal PR - #760
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new pull request #760: BP-20: Github workflow for bookkeeper proposals

[GitBox]

sijie opened a new pull request #760: BP-20: Github workflow for bookkeeper 
proposals
URL: https://github.com/apache/bookkeeper/pull/760
 
 
   Descriptions of the changes in this PR:
   
   Implementing a github workflow for bookkeeper proposals, so all the 
bookkeeper proposals can be reviewed as code changes, and they can be 
maintained in the same way as how we maintain documentation.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

sijie commented on issue #755: Issue 750: support ByteBuf, ByteBuffer, byte[] 
in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#issuecomment-346440293
 
 
   @ivankelly I addressed your comments. please take a look. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

sijie commented on a change in pull request #755: Issue 750: support ByteBuf, 
ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152649602
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
 ##
 @@ -41,7 +41,8 @@
 /**
  * Add entry asynchronously to an open ledger.
  *
- * @param data array of bytes to be written
+ * @param data a bytebuf to be written. The bytebuf's reference count will 
be decremented by 1 after the
 
 Review comment:
   I am not sure what do you expect to see about this. There is no ownership 
thing for this. The accurate statement is the bytebuf is decremented by 1 when 
the returned completablefuture is done. Because if the caller still has 
reference counts, the bytebuf is still 'owned' by the caller after the 
completable future is returned.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152645332
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   @ivankelly it is not a `move`. `retain` is to increment the reference count. 
there is no move here, as the caller don't really know how many reference 
counts are referencing the underneath buffer. 
   
   The whole idea behind `retainedIterator` is following what `ByteBuf` has and 
provide exactly same behavior as what people would expect from `ByteBuf`. There 
is already some sort of reference counting mechanisms that bookkeeper is using. 
We can just follow what it already has and have a consistent naming, semantic 
and behavior as `ByteBuf`. Consistent naming and semantic is more important to 
me.
   
   retain - https://netty.io/4.1/api/io/netty/buffer/ByteBuf.html#retain--
   retainedSlice - 
https://netty.io/4.1/api/io/netty/buffer/ByteBuf.html#retainedSlice-int-int-
   
   Anyway, let's move forward without `retainedIterator`. It is not a blocker 
for 4.6.0. We can circle back in future if it is needed.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

sijie commented on a change in pull request #755: Issue 750: support ByteBuf, 
ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152639133
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/LedgerEntryImpl.java
 ##
 @@ -85,6 +86,7 @@ public void setLength(long length) {
 }
 
 public void setEntryBuf(ByteBuf buf) {
+ReferenceCountUtil.release(entryBuf);
 
 Review comment:
   yes it is null safe.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #759: (WIP)Issue 695: add release notes for 4.6.0

[GitBox]

sijie commented on a change in pull request #759: (WIP)Issue 695: add release 
notes for 4.6.0
URL: https://github.com/apache/bookkeeper/pull/759#discussion_r152638408
 
 

 ##
 File path: site/docs/4.6.0/overview/releaseNotes.md
 ##
 @@ -2,16 +2,35 @@
 title: Apache BookKeeper 4.6.0 Release Notes
 ---
 
-[provide a summary of this release]
+This is the seventh release of BookKeeper as an Apache Top Level Project!
+
+The 4.6.0 release incorporates new fixes, improvements, and features since 
previous major release 4.5.0.
 
 Apache BookKeeper users are encouraged to upgrade to 4.6.0. The technical 
details of this release are summarized
 below.
 
 ## Highlights
 
-[List the highlights]
+The main features in 4.6.0 cover are around following areas:
+
+- Persistable bookie status
+- BookKeeper Admin REST API
+- New CreateLedger API
+- lifecycle components for managing components in bookie server
+- Introduce write FileInfo cache and read FileInfo cache
+- Use ByteBuf for entrylogger reads
+- Introduce Bookie Registration Manager for bookie server
+- Make bookie recovery work with recovering multiple bookies
+- Refine LedgerEntry interface and provide LedgerEntries interface
 
 Review comment:
   This should be part of "New BookKeeper API"


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #759: (WIP)Issue 695: add release notes for 4.6.0

[GitBox]

sijie commented on a change in pull request #759: (WIP)Issue 695: add release 
notes for 4.6.0
URL: https://github.com/apache/bookkeeper/pull/759#discussion_r152638318
 
 

 ##
 File path: site/docs/4.6.0/overview/releaseNotes.md
 ##
 @@ -2,16 +2,35 @@
 title: Apache BookKeeper 4.6.0 Release Notes
 ---
 
-[provide a summary of this release]
+This is the seventh release of BookKeeper as an Apache Top Level Project!
+
+The 4.6.0 release incorporates new fixes, improvements, and features since 
previous major release 4.5.0.
 
 Apache BookKeeper users are encouraged to upgrade to 4.6.0. The technical 
details of this release are summarized
 below.
 
 ## Highlights
 
-[List the highlights]
+The main features in 4.6.0 cover are around following areas:
+
+- Persistable bookie status
+- BookKeeper Admin REST API
+- New CreateLedger API
+- lifecycle components for managing components in bookie server
+- Introduce write FileInfo cache and read FileInfo cache
+- Use ByteBuf for entrylogger reads
 
 Review comment:
   Not sure we need to call this out separately. It should be one part of 
performance improvement.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #759: (WIP)Issue 695: add release notes for 4.6.0

[GitBox]

sijie commented on a change in pull request #759: (WIP)Issue 695: add release 
notes for 4.6.0
URL: https://github.com/apache/bookkeeper/pull/759#discussion_r152638194
 
 

 ##
 File path: site/docs/4.6.0/overview/releaseNotes.md
 ##
 @@ -2,16 +2,35 @@
 title: Apache BookKeeper 4.6.0 Release Notes
 ---
 
-[provide a summary of this release]
+This is the seventh release of BookKeeper as an Apache Top Level Project!
+
+The 4.6.0 release incorporates new fixes, improvements, and features since 
previous major release 4.5.0.
 
 Apache BookKeeper users are encouraged to upgrade to 4.6.0. The technical 
details of this release are summarized
 below.
 
 ## Highlights
 
-[List the highlights]
+The main features in 4.6.0 cover are around following areas:
+
+- Persistable bookie status
+- BookKeeper Admin REST API
+- New CreateLedger API
 
 Review comment:
   New BookKeeper API


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #712: Issue 544: Bootup cookie validation considers an empty journal to signify a new bookie

[GitBox]

sijie commented on issue #712: Issue 544: Bootup cookie validation considers an 
empty journal to signify a new bookie
URL: https://github.com/apache/bookkeeper/pull/712#issuecomment-346427007
 
 
   @jvrao ping?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie closed pull request #758: fix releases.md, add release date for 4.5.1 and clean up file

[GitBox]

sijie closed pull request #758: fix releases.md, add release date for 4.5.1 and 
clean up file
URL: https://github.com/apache/bookkeeper/pull/758
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/site/releases.md b/site/releases.md
index fab3296f5..d84d63b81 100644
--- a/site/releases.md
+++ b/site/releases.md
@@ -27,17 +27,13 @@ Client Guide | API docs
 
 ## News
 
-### [date] Release 4.6.0 available
-
-[INSERT SUMMARY]
-
-### XX November, 2017: Release 4.5.1 available
+### 22 November, 2017: Release 4.5.1 available
 
 This is the sixth release of BookKeeper as an Apache Top Level Project!
 
 The 4.5.1 release is a bugfix release.
 
-See [BookKeeper 4.5.0 Release Notes](../docs/4.5.0/overview/releaseNotes) for 
details.
+See [BookKeeper 4.5.1 Release Notes](../docs/4.5.1/overview/releaseNotes) for 
details.
 
 ### 10 August, 2017: Release 4.5.0 available
 


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #758: fix releases.md, add release date for 4.5.1 and clean up file

[GitBox]

sijie commented on issue #758: fix releases.md, add release date for 4.5.1 and 
clean up file
URL: https://github.com/apache/bookkeeper/pull/758#issuecomment-346425884
 
 
   lgtm +1


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] merlimat commented on a change in pull request #754: Issue #753: Allow option to disable data sync on journal

[GitBox]

merlimat commented on a change in pull request #754: Issue #753: Allow option 
to disable data sync on journal
URL: https://github.com/apache/bookkeeper/pull/754#discussion_r152632745
 
 

 ##
 File path: bookkeeper-server/conf/bk_server.conf
 ##
 @@ -296,6 +296,10 @@ journalDirectory=/tmp/bk-txn
 # Should we remove pages from page cache after force write
 # journalRemoveFromPageCache=false
 
+# Should the data be fsynced on journal before acknowledgment
 
 Review comment:
   Sure, I'll add more explicit docs on the risks of enabling the flag. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] merlimat commented on a change in pull request #754: Issue #753: Allow option to disable data sync on journal

[GitBox]

merlimat commented on a change in pull request #754: Issue #753: Allow option 
to disable data sync on journal
URL: https://github.com/apache/bookkeeper/pull/754#discussion_r152632489
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java
 ##
 @@ -914,9 +918,25 @@ public void run() {
 
forceWriteBatchEntriesStats.registerSuccessfulValue(toFlush.size());
 
forceWriteBatchBytesStats.registerSuccessfulValue(batchSize);
 
-forceWriteRequests.put(new 
ForceWriteRequest(logFile, logId, lastFlushPosition, toFlush,
-(lastFlushPosition > maxJournalSize), 
false));
-toFlush = new LinkedList();
+boolean shouldRolloverJournal = (lastFlushPosition 
> maxJournalSize);
+if (syncData) {
+// Trigger data sync to disk in the 
"Force-Write" thread. Callback will be triggered after data is committed to disk
+forceWriteRequests.put(new 
ForceWriteRequest(logFile, logId, lastFlushPosition, toFlush, 
shouldRolloverJournal, false));
+toFlush = new LinkedList();
+} else {
+// Data is already written on the file (though 
it might still be in the OS page-cache)
+lastLogMark.setCurLogMark(logId, 
lastFlushPosition);
+for (int i = 0; i < toFlush.size(); i++) {
+cbThreadPool.execute((QueueEntry) 
toFlush.get(i));
+}
+
+toFlush.clear();
+if (shouldRolloverJournal) {
+forceWriteRequests.put(new 
ForceWriteRequest(logFile, logId, lastFlushPosition,
+new LinkedList<>(), 
shouldRolloverJournal, false));
+}
+}
+
 
 Review comment:
   > Why not simply move the logic into JournalChnnel:forceWrite()?
   
   One reason is simplicity (the logic in the force-sync thread and request is 
already complicated, and I didn't want to add more in there). 
   
   The other is to avoid one additional context switch before sending the ack. 
If we say that the write is "good" (for whatever definition of "good" is 
configured in the bookie), then we should acknowledge immediately.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152628155
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   The reference count that a LedgerEntry has before iterator() is called 
belongs to LedgerEntries, because that is referring to the fact that 
LedgerEntries own those objects.
   
   With retainedIterator there is two options. a) the ownership of the 
reference is moved to the Iterator (like move semantics in c++) or b) a new 
Iterator object with a new reference count on the LedgerEntry is created.
   
   a) is a move, b) is a copy (albeit shallow).
   
   If retainedIterator is part of the API I would prefer the a) semantic, but 
this would imply that retainedIterator can only be called once. If it's b) I 
would prefer it be done with a Utility method. 
   
   And if retainedIterator is kept, rename it to retainingIterator. It should 
be an adjective.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] zhaijack opened a new pull request #759: (WIP)Issue 695: add release notes for 4.6.0

[GitBox]

zhaijack opened a new pull request #759: (WIP)Issue 695: add release notes for 
4.6.0
URL: https://github.com/apache/bookkeeper/pull/759
 
 
   Descriptions of the changes in this PR:
   add release notes for 4.6.0, this is still WIP.
   
   > ---
   > Be sure to do all of the following to help us incorporate your contribution
   > quickly and easily:
   > 
   > - [x] Make sure the PR title is formatted like:
   > `: Description of pull request`
   > `e.g. Issue 123: Description ...`
   > `e.g. BOOKKEEPER-1234: Description ...`
   > - [x] Make sure tests pass via `mvn clean apache-rat:check install 
findbugs:check`.
   > - [x] Replace `` in the title with the actual 
Issue/JIRA number.
   > 
   > ---
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

jiazhai commented on a change in pull request #755: Issue 750: support ByteBuf, 
ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152553438
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
 Review comment:
   For backward compat, we may not able to change this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152550963
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   > If you just only provide iterator(), you have to do two rounds of 
iterations. First round, you get an iterator to retrieve ledger entries, retain 
the reference count and add those entries to a list, then return a brand new 
iterator. Second round, your application will iterate over the new iterator. 
This introduces unnecessary complexity.
   
   Doesn't retainIterator need to iterate over the LedgerEntries anyhow to 
increase the ref counts.
   
   > it is more a convenient util function
   
   Which is why I think it would fit better in a Util class. 
   
   ```
   class LedgerEntriesUtils {
   public static Iterator retainIterator(Iterator 
iterator);
   }
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152549714
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   sure. "reference count" should goes with how caller uses it, not simply 
stays with LedgerEntries object. that is the semantic problem, you need to take 
account on how applications use it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152548916
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   why people need retainSlice for a bytebuf? A slice is essentially an 
iterator for the shared buffer. 
   
   There are use cases for this - for example, if you are building a service, 
which would cache LedgerEntries, there might be multiple readers will read 
those cached LedgerEntries, they would need different iterators on iterating 
the entries. The readers would retain the iterator, and release after they 
finish iteration. The cache will release its held reference when a 
LedgerEntries is evicted from the cache. A correct reference counting behavior 
should be provided when the caller attempts to create iterators.
   
   If you just only provide iterator(), you have to do two rounds of 
iterations. First round, you get an iterator to retrieve ledger entries, retain 
the reference count and add those entries to a list, then return a brand new 
iterator. Second round, your application will iterate over the new iterator. 
This introduces unnecessary complexity.
   
   again, if you are just viewing this from a single user application 
perspective, you don't need `retainIterator()`. but if you are thinking this 
from a shared use case, an iterator for a LedgerEntries is essentially a 
`slice` for a ByteBuf. The reasons why ByteBuf needs a `retainedSlice()` apply 
here.
   
   again, I see this can simplify a lot of things for applications. However I 
am not going to make a strong case here. I don't see it is a correctness 
debate, it is more a convenient util function. If you feel strong about that, I 
am fine with removing it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152545958
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   @ivankelly If you think this as `ByteBuf.slice()`, you are handing out a 
slice of the bytebuffer, you need to declare what would be the behavior for 
referencing count. Personally I feel having `iterator` and `retainIterator` 
will make a clear behavior what callers would expect from the interfaces. This 
is not a `correctness` debate here, it is more about what should be the 
reference counting semantic for the callers.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152544979
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   sure. if we make a clear comment on `iterator()`, I don't think 
retainIterator is needed. However if a caller wants to keep an iterator beyond 
the lifecycle of `LedgerEntries`, then a retainedIterator() is required. 
because there is no other simpler way to do that. So I would suggest us 
following the ByteBuf pattern to provide convenient methods for caller to uses. 
However I am not going to make a strong case for it. I am fine with removing it 
if you feel strong about that.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152542991
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
 
 Review comment:
   This method does not increment the reference count of ByteBuf ...


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152543344
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -35,10 +35,15 @@
 LedgerEntry getEntry(long entryId);
 
 /**
- * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * In this method, It does not increment the reference counts of ByteBuf 
for the entries in this LedgerEntries.
  * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
  *
- * @return the iterator of type LedgerEntry
+ * when iterator is called, you are handing out the entries, you may not 
know when the caller will
 
 Review comment:
   I don't think this is correct. You're not handing out the entries. You are 
handing out an iterator which will iterate over the set of entries held by the 
LedgerEntries. I don't think refcounting should come into the iterator part of 
it at all.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152542872
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   @sijie Ok. I'm ok with it, but I don't think it's needed. Your original 
comment seems to imply that the iterator itself owned the ref count of the 
entries, but I think it's better to view it as the LedgerEntries owning it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152541820
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/impl/LedgerEntriesImplTest.java
 ##
 @@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bookkeeper.client.impl;
+
+import static com.google.common.base.Charsets.UTF_8;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.Lists;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import java.util.Iterator;
+import java.util.List;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.junit.After;
+import org.junit.Test;
+
+/**
+ * Unit test for {@link LedgerEntriesImpl}.
+ */
+public class LedgerEntriesImplTest {
+private final int entryNumber = 7;
+private LedgerEntriesImpl ledgerEntriesImpl;
+private final List entryList = Lists.newArrayList();
+
+// content for each entry
+private final long ledgerId = 1234L;
+private final long entryId = 5678L;
+private final long length = 9876L;
+private final byte[] dataBytes = "test-ledger-entry-impl".getBytes(UTF_8);
+
+public LedgerEntriesImplTest () {
+for(int i = 0; i < entryNumber; i++) {
+entryList.add(LedgerEntryImpl.create(ledgerId + i,
+entryId + i,
+length + i,
+Unpooled.wrappedBuffer(dataBytes)));
+}
+
+ledgerEntriesImpl = LedgerEntriesImpl.create(entryList);
+}
+
+@After
+public void tearDown() {
+ledgerEntriesImpl.close();
+
+try {
+ledgerEntriesImpl.getEntry(entryId);
+fail("should fail getEntry after close");
+} catch (NullPointerException e) {
+
 
 Review comment:
   oh. not noticed this one before.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on issue #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on issue #727: Issue 693: add interface and implementation 
of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#issuecomment-346325678
 
 
   @zhaijack it's good to have it here too, so that when we look at the git 
log, we don't have to jump back to the issue to figure stuff out. It's often 
the case also that the final patch diverges significantly from the original 
issue, so it's good to capture that and summarize at the end.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520301
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
+/**
+ * Exposes this entry's data as an NIO {@link ByteBuffer}. The returned 
buffer
+ * shares the content with this underneath bytebuf (which you can get it 
by {@link #getEntryBuffer()}),
+ * while changing the position and limit of the returned NIO buffer does 
not affect the indexes and
+ * marks of this underneath buffer.  This method is identical
+ * to {@code entry.getEntryBuffer().nioBuffer()}. This method does not
+ * modify {@code readerIndex} or {@code writerIndex} of the underneath 
bytebuf.
+ */
+ByteBuffer getNioBuffer();
+
 /**
  * Return the internal buffer that contains the entry payload.
  *
+ * This call doesn't retain any reference on the underneath bytebuf. If 
you want to use the bytebuf
+ * after the entry is released (via {@link #close()}, the caller must 
retain the references of the bytebuf.
 
 Review comment:
   ..., the caller must call retain on the ByteBuf.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520412
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
 Review comment:
   Maybe rename to getEntryBytes()


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152522492
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/LedgerEntryImpl.java
 ##
 @@ -85,6 +86,7 @@ public void setLength(long length) {
 }
 
 public void setEntryBuf(ByteBuf buf) {
+ReferenceCountUtil.release(entryBuf);
 
 Review comment:
   is this null safe?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152522259
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
 ##
 @@ -56,6 +57,30 @@
 return append(Unpooled.wrappedBuffer(data));
 }
 
+/**
+ * Add an entry asynchronously to an open ledger.
+ *
+ * @param data array of bytes to be written.
+ * @return a completable future represents the add result, in case of 
success the future returns the entry id
+ * of this newly appended entry.
 
 Review comment:
   Indent


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520795
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -21,12 +21,17 @@
 package org.apache.bookkeeper.client.api;
 
 import io.netty.buffer.ByteBuf;
+import java.nio.ByteBuffer;
 import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public;
 import org.apache.bookkeeper.common.annotation.InterfaceStability.Unstable;
 
 /**
  * An entry.
  *
+ * The entry implementation might be holding references to byte buffers 
under the hood. The users holding the
 
 Review comment:
   The entry implementation may hold references to byte buffers under the hood.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r15252
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
 ##
 @@ -41,7 +41,8 @@
 /**
  * Add entry asynchronously to an open ledger.
  *
- * @param data array of bytes to be written
+ * @param data a bytebuf to be written. The bytebuf's reference count will 
be decremented by 1 after the
 
 Review comment:
   I think it would be better to restate this, or to also state, that the 
append call passes ownership to of the bytebuf to the handle.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152519495
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
+/**
+ * Exposes this entry's data as an NIO {@link ByteBuffer}. The returned 
buffer
+ * shares the content with this underneath bytebuf (which you can get it 
by {@link #getEntryBuffer()}),
 
 Review comment:
   underlying ByteBuf


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520927
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -21,12 +21,17 @@
 package org.apache.bookkeeper.client.api;
 
 import io.netty.buffer.ByteBuf;
+import java.nio.ByteBuffer;
 import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public;
 import org.apache.bookkeeper.common.annotation.InterfaceStability.Unstable;
 
 /**
  * An entry.
 
 Review comment:
   An entry in a Ledger.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152519254
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
 
 Review comment:
   as a byte array.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520871
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -21,12 +21,17 @@
 package org.apache.bookkeeper.client.api;
 
 import io.netty.buffer.ByteBuf;
+import java.nio.ByteBuffer;
 import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public;
 import org.apache.bookkeeper.common.annotation.InterfaceStability.Unstable;
 
 /**
  * An entry.
  *
+ * The entry implementation might be holding references to byte buffers 
under the hood. The users holding the
+ * references to the instances of this class, are responsible for calling 
{@link LedgerEntry#close()} to release
+ * resources held by the instances.
 
 Review comment:
   held by the entry instances.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152519840
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
+/**
+ * Exposes this entry's data as an NIO {@link ByteBuffer}. The returned 
buffer
+ * shares the content with this underneath bytebuf (which you can get it 
by {@link #getEntryBuffer()}),
 
 Review comment:
   Maybe add a @link for ByteBuf


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152522328
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteHandle.java
 ##
 @@ -56,6 +57,30 @@
 return append(Unpooled.wrappedBuffer(data));
 }
 
+/**
+ * Add an entry asynchronously to an open ledger.
+ *
+ * @param data array of bytes to be written.
+ * @return a completable future represents the add result, in case of 
success the future returns the entry id
+ * of this newly appended entry.
+ */
+default CompletableFuture append(byte[] data) {
+return append(Unpooled.wrappedBuffer(data));
+}
+
+/**
+ * Add an entry asynchronously to an open ledger.
+ *
+ * @param data array of bytes to be written.
+ * @param offset the offset in the bytes array.
+ * @param length the length of the bytes to be appended.
+ * @return a completable future represents the add result, in case of 
success the future returns the entry id
+ * of this newly appended entry.
 
 Review comment:
   Intent


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152519764
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
+/**
+ * Exposes this entry's data as an NIO {@link ByteBuffer}. The returned 
buffer
+ * shares the content with this underneath bytebuf (which you can get it 
by {@link #getEntryBuffer()}),
+ * while changing the position and limit of the returned NIO buffer does 
not affect the indexes and
+ * marks of this underneath buffer.  This method is identical
+ * to {@code entry.getEntryBuffer().nioBuffer()}. This method does not
+ * modify {@code readerIndex} or {@code writerIndex} of the underneath 
bytebuf.
 
 Review comment:
   underlying ByteBuf


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520550
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
+/**
+ * Exposes this entry's data as an NIO {@link ByteBuffer}. The returned 
buffer
+ * shares the content with this underneath bytebuf (which you can get it 
by {@link #getEntryBuffer()}),
+ * while changing the position and limit of the returned NIO buffer does 
not affect the indexes and
+ * marks of this underneath buffer.  This method is identical
+ * to {@code entry.getEntryBuffer().nioBuffer()}. This method does not
+ * modify {@code readerIndex} or {@code writerIndex} of the underneath 
bytebuf.
+ */
+ByteBuffer getNioBuffer();
 
 Review comment:
   getEntryNioBuffer to be consistent?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152520211
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -55,15 +60,28 @@
 long getLength();
 
 /**
- * Returns the content of the entry.
+ * Returns the content of the entry into a byte array.
  *
  * @return the content of the entry
  */
 byte[] getEntry();
 
+/**
+ * Exposes this entry's data as an NIO {@link ByteBuffer}. The returned 
buffer
+ * shares the content with this underneath bytebuf (which you can get it 
by {@link #getEntryBuffer()}),
+ * while changing the position and limit of the returned NIO buffer does 
not affect the indexes and
+ * marks of this underneath buffer.  This method is identical
+ * to {@code entry.getEntryBuffer().nioBuffer()}. This method does not
+ * modify {@code readerIndex} or {@code writerIndex} of the underneath 
bytebuf.
+ */
+ByteBuffer getNioBuffer();
+
 /**
  * Return the internal buffer that contains the entry payload.
  *
+ * This call doesn't retain any reference on the underneath bytebuf. If 
you want to use the bytebuf
 
 Review comment:
   This call doesn't change the reference count on the returned ByteBuf.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

ivankelly commented on a change in pull request #755: Issue 750: support 
ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152521440
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteAdvHandle.java
 ##
 @@ -45,12 +45,36 @@
  *
  * @param entryId entryId to be added
  * @param data array of bytes to be written
- * @return an handle to the result, in case of success it will return the 
same value of param entryId
+ * @return an handle to the result, in case of success it will return the 
same value of param entryId.
 
 Review comment:
   It's recommended to have no full stop at the end of @param, @return and 
@throws
   
   http://www.oracle.com/technetwork/articles/java/index-137868.html


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on issue #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on issue #727: Issue 693: add interface and implementation of 
LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#issuecomment-346308299
 
 
   @ivankelly @sijie  updated.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152518536
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   Thanks, @ivankelly @sijie , will put some of your comments here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #738: Issue 731: refine LedgerEntry interface and implementation

[GitBox]

ivankelly commented on a change in pull request #738: Issue 731: refine 
LedgerEntry interface and implementation
URL: https://github.com/apache/bookkeeper/pull/738#discussion_r152518358
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -56,25 +55,31 @@
 long getLength();
 
 /**
- * Returns the content of the entry. This method can be called only once. 
While using v2 wire protocol this method
- * will automatically release the internal ByteBuf.
+ * Returns the content of the entry.
  *
  * @return the content of the entry
- * @throws IllegalStateException if this method is called twice
  */
 byte[] getEntry();
 
 /**
 
 Review comment:
   ah, excellent, will check it out there.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152516308
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
 
 Review comment:
   thanks, will change it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on issue #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on issue #727: Issue 693: add interface and implementation of 
LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#issuecomment-346301408
 
 
   @ivan, Thanks for the comments, the reason is described  in  the issue, will 
move it here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152515790
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   that was my review comment - 
https://github.com/apache/bookkeeper/pull/727#discussion_r151218118


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #677: Issue 659: Fix Checkpoint logic in SortedLedgerStorage

[GitBox]

ivankelly commented on a change in pull request #677: Issue 659: Fix Checkpoint 
logic in SortedLedgerStorage
URL: https://github.com/apache/bookkeeper/pull/677#discussion_r152515374
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageCheckpointTest.java
 ##
 @@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.TimeUnit;
+import lombok.Data;
+import lombok.EqualsAndHashCode;
+import lombok.RequiredArgsConstructor;
+import lombok.ToString;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.bookie.CheckpointSource.Checkpoint;
+import org.apache.bookkeeper.meta.LedgerManager;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Test {@link SortedLedgerStorage}.
+ */
+@Slf4j
+public class SortedLedgerStorageCheckpointTest extends LedgerStorageTestBase {
+
+@Data
+@RequiredArgsConstructor
+@ToString
+@EqualsAndHashCode
+private static class TestCheckpoint implements Checkpoint {
+
+private final long offset;
+
+@Override
+public int compareTo(Checkpoint o) {
+if (Checkpoint.MAX == o) {
+return -1;
+}
+
+TestCheckpoint other = (TestCheckpoint) o;
+return Long.compare(offset, other.offset);
+}
+
+}
+
+@RequiredArgsConstructor
+private static class TestCheckpointSource implements CheckpointSource {
+
+private long currentOffset = 0;
+
+void advanceOffset(long numBytes) {
+currentOffset += numBytes;
+}
+
+@Override
+public Checkpoint newCheckpoint() {
+TestCheckpoint cp = new TestCheckpoint(currentOffset);
+log.info("New checkpoint : {}", cp);
+return cp;
+}
+
+@Override
+public void checkpointComplete(Checkpoint checkpoint, boolean compact)
+throws IOException {
+log.info("Complete checkpoint : {}", checkpoint);
+}
+}
+
+private SortedLedgerStorage storage;
+private Checkpointer checkpointer;
+private final LinkedBlockingQueue checkpoints;
+private final TestCheckpointSource checkpointSrc = new 
TestCheckpointSource();
+
+public SortedLedgerStorageCheckpointTest() {
+super();
+conf.setEntryLogSizeLimit(1);
+this.checkpoints = new LinkedBlockingQueue<>();
+}
+
+@Before
+@Override
+public void setUp() throws Exception {
+super.setUp();
+
+// initial checkpoint
+
+this.storage = new SortedLedgerStorage();
+this.checkpointer = checkpoint -> storage.getScheduler().submit(() -> {
+log.info("Checkpoint the storage at {}", checkpoint);
+try {
+storage.checkpoint(checkpoint);
+checkpoints.add(checkpoint);
+} catch (IOException e) {
+log.error("Failed to checkpoint at {}", checkpoint, e);
+}
+});
+this.storage.initialize(
+conf,
+mock(LedgerManager.class),
+ledgerDirsManager,
+ledgerDirsManager,
+checkpointSrc,
+checkpointer,
+NullStatsLogger.INSTANCE);
+}
+
+@After
+@Override
+public void tearDown() throws Exception {
+if (null != storage) {
+storage.shutdown();
+}
+super.tearDown();
+}
+
+ByteBuf prepareEntry(long ledgerId, long entryId) {
+ByteBuf entry = Unpooled.buffer(4 * Long.BYTES);
+// ledger id, entry id, lac
+entry.writeLong(ledgerId);
+entry.writeLong(entryId);
+entry.writeLong(entryId - 

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152515198
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   @ivankelly it was me requesting adding this method. it is following the 
pattern that ByteBuf has for creating a `slice` of the buffer. slice: 
https://netty.io/4.0/api/io/netty/buffer/ByteBuf.html#slice-- and retainedSlice 
https://netty.io/4.1/api/io/netty/buffer/ByteBuf.html#retainedSlice--.  This 
provides a shortcut for callers that want to retain refcnt for all the entries. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #738: Issue 731: refine LedgerEntry interface and implementation

[GitBox]

sijie commented on a change in pull request #738: Issue 731: refine LedgerEntry 
interface and implementation
URL: https://github.com/apache/bookkeeper/pull/738#discussion_r152513783
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -56,25 +55,31 @@
 long getLength();
 
 /**
- * Returns the content of the entry. This method can be called only once. 
While using v2 wire protocol this method
- * will automatically release the internal ByteBuf.
+ * Returns the content of the entry.
  *
  * @return the content of the entry
- * @throws IllegalStateException if this method is called twice
  */
 byte[] getEntry();
 
 /**
 
 Review comment:
   @ivankelly - This call doesn't retain any reference of the underneath 
bytebuf. if a caller releases the entry by #close(), it will release the 
underneath bytebuf. If the caller want to use the bytebuf beyond the lifecycle 
of this entry, it has to retain the buf by itself and then be responsible for 
releasing its retained refcnt. I have improved the comment in my subsequent 
change at #755 . 
   
   https://github.com/apache/bookkeeper/pull/755/files


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

jiazhai commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152513610
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/impl/LedgerEntriesImplTest.java
 ##
 @@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bookkeeper.client.impl;
+
+import static com.google.common.base.Charsets.UTF_8;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.Lists;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import java.util.Iterator;
+import java.util.List;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.junit.After;
+import org.junit.Test;
+
+/**
+ * Unit test for {@link LedgerEntriesImpl}.
+ */
+public class LedgerEntriesImplTest {
+private final int entryNumber = 7;
+private LedgerEntriesImpl ledgerEntriesImpl;
+private final List entryList = Lists.newArrayList();
+
+// content for each entry
+private final long ledgerId = 1234L;
+private final long entryId = 5678L;
+private final long length = 9876L;
+private final byte[] dataBytes = "test-ledger-entry-impl".getBytes(UTF_8);
+
+public LedgerEntriesImplTest () {
+for(int i = 0; i < entryNumber; i++) {
+entryList.add(LedgerEntryImpl.create(ledgerId + i,
+entryId + i,
+length + i,
+Unpooled.wrappedBuffer(dataBytes)));
+}
+
+ledgerEntriesImpl = LedgerEntriesImpl.create(entryList);
+}
+
+@After
+public void tearDown() {
+ledgerEntriesImpl.close();
+
+try {
+ledgerEntriesImpl.getEntry(entryId);
+fail("should fail getEntry after close");
+} catch (NullPointerException e) {
+
+}
+
+try {
+ledgerEntriesImpl.iterator();
+fail("should fail iterator after close");
+} catch (NullPointerException e) {
+
+}
+
+try {
+ledgerEntriesImpl.retainIterator();
+fail("should fail retainIterator after close");
+} catch (NullPointerException e) {
+
+}
+}
+
+@Test
+public void testGetEntry() {
+for(int i = 0; i < entryNumber; i ++) {
+LedgerEntry entry = ledgerEntriesImpl.getEntry(entryId + i);
+assertEquals(entryList.get(i).getLedgerId(),  entry.getLedgerId());
+assertEquals(entryList.get(i).getEntryId(),  entry.getEntryId());
+assertEquals(entryList.get(i).getLength(),  entry.getLength());
+
+ByteBuf buf = entry.getEntryBuffer();
+byte[]  content = new byte[buf.readableBytes()];
+buf.readBytes(content);
+assertArrayEquals(dataBytes, content);
+
+assertEquals(1, entry.getEntryBuffer().refCnt());
+}
+
+try {
+LedgerEntry entry = ledgerEntriesImpl.getEntry(entryId - 1);
+fail("Should get IndexOutOfBoundsException");
+} catch (IndexOutOfBoundsException e) {
+
+}
+
+try {
+LedgerEntry entry = ledgerEntriesImpl.getEntry(entryId + 
entryNumber);
+fail("Should get IndexOutOfBoundsException");
+} catch (IndexOutOfBoundsException e) {
+
+}
+}
+
+@Test
+public void testIterator() {
+Iterator entryIterator = ledgerEntriesImpl.iterator();
+entryIterator.forEachRemaining(ledgerEntry -> assertEquals(1, 
ledgerEntry.getEntryBuffer().refCnt()));
+}
+
+@Test
+public void testRetainIterator() {
+Iterator entryIterator = 
ledgerEntriesImpl.retainIterator();
+entryIterator.forEachRemaining(ledgerEntry -> assertEquals(2, 
ledgerEntry.getEntryBuffer().refCnt()));
+entryIterator.forEachRemaining(ledgerEntry -> 
ledgerEntry.getEntryBuffer().release());
 
 Review comment:
   Thanks. will change this


This is an automated message from the Apache Git Service.
To respond 

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152512523
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/impl/LedgerEntriesImplTest.java
 ##
 @@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bookkeeper.client.impl;
+
+import static com.google.common.base.Charsets.UTF_8;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.Lists;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import java.util.Iterator;
+import java.util.List;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.junit.After;
+import org.junit.Test;
+
+/**
+ * Unit test for {@link LedgerEntriesImpl}.
+ */
+public class LedgerEntriesImplTest {
+private final int entryNumber = 7;
+private LedgerEntriesImpl ledgerEntriesImpl;
+private final List entryList = Lists.newArrayList();
+
+// content for each entry
+private final long ledgerId = 1234L;
+private final long entryId = 5678L;
+private final long length = 9876L;
+private final byte[] dataBytes = "test-ledger-entry-impl".getBytes(UTF_8);
+
+public LedgerEntriesImplTest () {
+for(int i = 0; i < entryNumber; i++) {
+entryList.add(LedgerEntryImpl.create(ledgerId + i,
+entryId + i,
+length + i,
+Unpooled.wrappedBuffer(dataBytes)));
+}
+
+ledgerEntriesImpl = LedgerEntriesImpl.create(entryList);
+}
+
+@After
+public void tearDown() {
+ledgerEntriesImpl.close();
+
+try {
+ledgerEntriesImpl.getEntry(entryId);
+fail("should fail getEntry after close");
+} catch (NullPointerException e) {
+
 
 Review comment:
   +1


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

ivankelly commented on a change in pull request #727: Issue 693: add interface 
and implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152511643
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java
 ##
 @@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.client.api;
+
+import java.util.Iterator;
+
+/**
+ * Interface to wrap the entries.
+ *
+ * @since 4.6
+ */
+public interface LedgerEntries extends AutoCloseable {
+
+/**
+ * Gets a specific LedgerEntry by entryId.
+ *
+ * @param entryId the LedgerEntry id
+ * @return the LedgerEntry, null if no LedgerEntry with such entryId.
+ */
+LedgerEntry getEntry(long entryId);
+
+/**
+ * In this, It does not retain the ByteBuf references for the entries in 
this LedgerEntries.
+ * The caller who calls {@link #iterator()} should be careful for not 
releasing the references.
+ *
+ * @return the iterator of type LedgerEntry
+ */
+Iterator iterator();
+
+/**
+ * In this, It retains the ByteBuf references for the entries in this 
LedgerEntries.
+ * The caller who calls {@link #retainIterator()} is responsible for 
releasing the retained references.
+ *
+ * @return the iterator of type LedgerEntry that has been retained
+ */
+Iterator retainIterator();
 
 Review comment:
   Why? Why not just state that that for iterator(), the LedgerEntrys are owned 
by the LedgerEntries instance, and if you want to retain the entry beyond the 
lifetime of the LedgerEntries instance, you should call retain on the entry 
manually.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #738: Issue 731: refine LedgerEntry interface and implementation

[GitBox]

ivankelly commented on a change in pull request #738: Issue 731: refine 
LedgerEntry interface and implementation
URL: https://github.com/apache/bookkeeper/pull/738#discussion_r152510710
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntry.java
 ##
 @@ -56,25 +55,31 @@
 long getLength();
 
 /**
- * Returns the content of the entry. This method can be called only once. 
While using v2 wire protocol this method
- * will automatically release the internal ByteBuf.
+ * Returns the content of the entry.
  *
  * @return the content of the entry
- * @throws IllegalStateException if this method is called twice
  */
 byte[] getEntry();
 
 /**
 
 Review comment:
   What happens to the ownership of the returned ByteBuf? Is the caller 
responsible for releasing it? @sijie @zhaijack 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #758: fix releases.md, add release date for 4.5.1 and clean up file

[GitBox]

sijie commented on a change in pull request #758: fix releases.md, add release 
date for 4.5.1 and clean up file
URL: https://github.com/apache/bookkeeper/pull/758#discussion_r152510397
 
 

 ##
 File path: site/.ruby-version
 ##
 @@ -0,0 +1 @@
+2.4.1
 
 Review comment:
   ?? what is this for?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152509013
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/impl/LedgerEntriesImplTest.java
 ##
 @@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bookkeeper.client.impl;
+
+import static com.google.common.base.Charsets.UTF_8;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.Lists;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import java.util.Iterator;
+import java.util.List;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.junit.After;
+import org.junit.Test;
+
+/**
+ * Unit test for {@link LedgerEntriesImpl}.
+ */
+public class LedgerEntriesImplTest {
+private final int entryNumber = 7;
+private LedgerEntriesImpl ledgerEntriesImpl;
+private final List entryList = Lists.newArrayList();
+
+// content for each entry
+private final long ledgerId = 1234L;
+private final long entryId = 5678L;
+private final long length = 9876L;
+private final byte[] dataBytes = "test-ledger-entry-impl".getBytes(UTF_8);
+
+public LedgerEntriesImplTest () {
+for(int i = 0; i < entryNumber; i++) {
+entryList.add(LedgerEntryImpl.create(ledgerId + i,
+entryId + i,
+length + i,
+Unpooled.wrappedBuffer(dataBytes)));
+}
+
+ledgerEntriesImpl = LedgerEntriesImpl.create(entryList);
+}
+
+@After
+public void tearDown() {
+ledgerEntriesImpl.close();
+
+try {
+ledgerEntriesImpl.getEntry(entryId);
+fail("should fail getEntry after close");
+} catch (NullPointerException e) {
+
 
 Review comment:
   I would suggest not leaving an empty line. you can add some comments like 
"expected behavior"


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries

[GitBox]

sijie commented on a change in pull request #727: Issue 693: add interface and 
implementation of LedgerEntries
URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152509013
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/impl/LedgerEntriesImplTest.java
 ##
 @@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.bookkeeper.client.impl;
+
+import static com.google.common.base.Charsets.UTF_8;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.Lists;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import java.util.Iterator;
+import java.util.List;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.junit.After;
+import org.junit.Test;
+
+/**
+ * Unit test for {@link LedgerEntriesImpl}.
+ */
+public class LedgerEntriesImplTest {
+private final int entryNumber = 7;
+private LedgerEntriesImpl ledgerEntriesImpl;
+private final List entryList = Lists.newArrayList();
+
+// content for each entry
+private final long ledgerId = 1234L;
+private final long entryId = 5678L;
+private final long length = 9876L;
+private final byte[] dataBytes = "test-ledger-entry-impl".getBytes(UTF_8);
+
+public LedgerEntriesImplTest () {
+for(int i = 0; i < entryNumber; i++) {
+entryList.add(LedgerEntryImpl.create(ledgerId + i,
+entryId + i,
+length + i,
+Unpooled.wrappedBuffer(dataBytes)));
+}
+
+ledgerEntriesImpl = LedgerEntriesImpl.create(entryList);
+}
+
+@After
+public void tearDown() {
+ledgerEntriesImpl.close();
+
+try {
+ledgerEntriesImpl.getEntry(entryId);
+fail("should fail getEntry after close");
+} catch (NullPointerException e) {
+
 
 Review comment:
   nit: I would suggest not leaving an empty line. you can add some comments 
like "expected behavior"


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie closed pull request #752: Add release 4.6.0 to releases menu

[GitBox]

sijie closed pull request #752: Add release 4.6.0 to releases menu
URL: https://github.com/apache/bookkeeper/pull/752
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/site/_config.yml b/site/_config.yml
index 7c305e4b0..302c2e4d4 100644
--- a/site/_config.yml
+++ b/site/_config.yml
@@ -10,6 +10,7 @@ twitter_url: https://twitter.com/asfbookkeeper
 livereload: true
 
 versions:
+- "4.6.0"
 # [next_version_placehodler]
 - "4.5.1"
 - "4.5.0"
@@ -26,8 +27,8 @@ archived_versions:
 - "4.2.0"
 - "4.1.0"
 - "4.0.0"
-latest_version: "4.6.0-SNAPSHOT"
-latest_release: "4.5.1"
+latest_version: "4.7.0-SNAPSHOT"
+latest_release: "4.6.0"
 stable_release: "4.5.0"
 distributedlog_version: "0.5.0"
 
diff --git a/site/docs/4.5.1/overview/overview.md 
b/site/docs/4.5.1/overview/overview.md
index dba92dd25..9bef23255 100644
--- a/site/docs/4.5.1/overview/overview.md
+++ b/site/docs/4.5.1/overview/overview.md
@@ -1,5 +1,5 @@
 ---
-title: Apache BookKeeper 4.5.1 Documentation 
+title: Apache BookKeeper 4.5.1
 ---
 
-4.5.0
+4.6.0
 
 
 
@@ -36,12 +36,12 @@ If you're using [Gradle](https://gradle.org/), add this to 
your [`build.gradle`]
 
 ```groovy
 dependencies {
-compile group: 'org.apache.bookkeeper', name: 'bookkeeper-server', 
version: '4.5.0'
+compile group: 'org.apache.bookkeeper', name: 'bookkeeper-server', 
version: '4.6.0'
 }
 
 // Alternatively:
 dependencies {
-compile 'org.apache.bookkeeper:bookkeeper-server:4.5.0'
+compile 'org.apache.bookkeeper:bookkeeper-server:4.6.0'
 }
 ```
 
diff --git a/site/docs/4.6.0/deployment/dcos.md 
b/site/docs/4.6.0/deployment/dcos.md
index 3e174384e..ee3c956fd 100644
--- a/site/docs/4.6.0/deployment/dcos.md
+++ b/site/docs/4.6.0/deployment/dcos.md
@@ -137,6 +137,6 @@ You can shut down and uninstall the `bookkeeper` from DC/OS 
at any time using th
 
 ```shell
 $ dcos package uninstall bookkeeper
-Uninstalled package [bookkeeper] version [4.5.0]
+Uninstalled package [bookkeeper] version [4.6.0]
 Thank you for using bookkeeper.
 ```
diff --git a/site/docs/4.6.0/deployment/kubernetes.md 
b/site/docs/4.6.0/deployment/kubernetes.md
index f65172112..0f113169e 100644
--- a/site/docs/4.6.0/deployment/kubernetes.md
+++ b/site/docs/4.6.0/deployment/kubernetes.md
@@ -1,4 +1,181 @@
 ---
-title: Deploying BookKeeper on Kubernetes
+title: Deploying Apache BookKeeper on Kubernetes
+tags: [Kubernetes, Google Container Engine]
 logo: img/kubernetes-logo.png
 ---
+
+Apache BookKeeper can be easily deployed in 
[Kubernetes](https://kubernetes.io/) clusters. The managed clusters on [Google 
Container Engine](https://cloud.google.com/compute/) is the most convenient way.
+
+The deployment method shown in this guide relies on [YAML](http://yaml.org/) 
definitions for Kubernetes 
[resources](https://kubernetes.io/docs/resources-reference/v1.6/). The 
[`kubernetes`](https://github.com/apache/bookkeeper/tree/master/deploy/kubernetes)
 subdirectory holds resource definitions for:
+
+* A three-node ZooKeeper cluster
+* A BookKeeper cluster with a bookie runs on each node.
+
+## Setup on Google Container Engine
+
+To get started, get source code of 
[`kubernetes`](https://github.com/apache/bookkeeper/tree/master/deploy/kubernetes)
 from github by git clone.
+
+If you'd like to change the number of bookies,  or ZooKeeper nodes in your 
BookKeeper cluster, modify the `replicas` parameter in the `spec` section of 
the appropriate 
[`Deployment`](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/)
 or 
[`StatefulSet`](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/)
 resource.
+
+[Google Container Engine](https://cloud.google.com/container-engine) (GKE) 
automates the creation and management of Kubernetes clusters in [Google Compute 
Engine](https://cloud.google.com/compute/) (GCE).
+
+### Prerequisites
+
+To get started, you'll need:
+
+* A Google Cloud Platform account, which you can sign up for at 
[cloud.google.com](https://cloud.google.com)
+* An existing Cloud Platform project
+* The [Google Cloud SDK](https://cloud.google.com/sdk/downloads) (in 
particular the [`gcloud`](https://cloud.google.com/sdk/gcloud/) and 
[`kubectl`]() tools).
+
+### Create a new Kubernetes cluster
+
+You can create a new GKE cluster using the [`container clusters 
create`](https://cloud.google.com/sdk/gcloud/reference/container/clusters/create)
 command for `gcloud`. This command enables you to specify the number of nodes 
in the cluster, the machine types of those nodes, and more.
+
+As an example, we'll create a new GKE cluster for Kubernetes version 
[1.6.4](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md#v164) 
in the 
[us-central1-a](https://cloud.google.com/compute/docs/regions-zones/regions-zones#available)
 zone. 

[GitHub] sijie commented on a change in pull request #755: Issue 750: support ByteBuf, ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle

[GitBox]

sijie commented on a change in pull request #755: Issue 750: support ByteBuf, 
ByteBuffer, byte[] in both WriteHandle, WriteAdvHandle, ReadHandle
URL: https://github.com/apache/bookkeeper/pull/755#discussion_r152492194
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/WriteAdvHandle.java
 ##
 @@ -45,12 +45,36 @@
  *
  * @param entryId entryId to be added
  * @param data array of bytes to be written
- * @return an handle to the result, in case of success it will return the 
same value of param entryId
+ * @return an handle to the result, in case of success it will return the 
same value of param entryId.
  */
 default CompletableFuture write(final long entryId, final ByteBuffer 
data) {
 return write(entryId, Unpooled.wrappedBuffer(data));
 }
 
+/**
+ * Add entry asynchronously to an open ledger.
+ *
+ * @param entryId entryId to be added.
+ * @param data array of bytes to be written
 
 Review comment:
   created #757 for future improvements.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed issue #732: Release 4.5.1: update docker image file after 4.5.1 is released

[GitBox]

eolivelli closed issue #732: Release 4.5.1: update docker image file after 
4.5.1 is released
URL: https://github.com/apache/bookkeeper/issues/732
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #757: Improve javadoc for public interfaces that takes user input

[GitBox]

sijie commented on issue #757: Improve javadoc for public interfaces that takes 
user input
URL: https://github.com/apache/bookkeeper/issues/757#issuecomment-346274613
 
 
   This is from a discussion at #755 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #757: Improve javadoc for public interfaces that takes user input

[GitBox]

sijie opened a new issue #757: Improve javadoc for public interfaces that takes 
user input
URL: https://github.com/apache/bookkeeper/issues/757
 
 
   
   
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   per review comment at 
[#755](https://github.com/apache/bookkeeper/pull/755#discussion_r152485557), we 
need to document how an implementation of an interface will take the user 
input. so users will be aware of what to do regarding to the objects, buffers 
passed into an method.
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   *nice-to-have*
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #754: Issue #753: Allow option to disable data sync on journal

[GitBox]

sijie commented on a change in pull request #754: Issue #753: Allow option to 
disable data sync on journal
URL: https://github.com/apache/bookkeeper/pull/754#discussion_r152491411
 
 

 ##
 File path: bookkeeper-server/conf/bk_server.conf
 ##
 @@ -296,6 +296,10 @@ journalDirectory=/tmp/bk-txn
 # Should we remove pages from page cache after force write
 # journalRemoveFromPageCache=false
 
+# Should the data be fsynced on journal before acknowledgment
 
 Review comment:
   fyi, I created an issue #756 (we need some guidelines on providing 
guidelines for a feature introduced in a release).


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #756: Introduce annotations for features / configuration settings

[GitBox]

sijie opened a new issue #756: Introduce annotations for features / 
configuration settings
URL: https://github.com/apache/bookkeeper/issues/756
 
 
   
   
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   We have `InterfaceAudience` and `InterfaceStability` for defining the 
audience and stablity for a public interface. We need similar annotations for 
features. The features are mostly likely exposed by configuration settings. 
   
   These annotations should provide informations like:
   
   - when a given feature was introduced?
   - what is the stability for a given feature?
   
   These annotation can be used for generating documentation and configuration 
and provide guideline for users.
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   *nice-to-have*
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services