[GitHub] sijie commented on a change in pull request #1108: Replace DoubleByteBuf with CompositeByteBuf because of perf regression with Netty > 4.1.12

[GitBox]

sijie commented on a change in pull request #1108: Replace DoubleByteBuf with 
CompositeByteBuf because of perf regression with Netty > 4.1.12
URL: https://github.com/apache/bookkeeper/pull/1108#discussion_r165578182
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/DoubleByteBuf.java
 ##
 @@ -17,447 +17,20 @@
 */
 package org.apache.bookkeeper.util;
 
-import io.netty.buffer.AbstractReferenceCountedByteBuf;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufAllocator;
-import io.netty.buffer.PooledByteBufAllocator;
-import io.netty.buffer.Unpooled;
-import io.netty.util.Recycler;
-import io.netty.util.Recycler.Handle;
-import io.netty.util.ResourceLeakDetector;
-import io.netty.util.ResourceLeakDetectorFactory;
-import io.netty.util.ResourceLeakTracker;
+import io.netty.buffer.CompositeByteBuf;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStream;
-import java.lang.reflect.Constructor;
-import java.nio.ByteBuffer;
-import java.nio.ByteOrder;
-import java.nio.channels.FileChannel;
-import java.nio.channels.GatheringByteChannel;
-import java.nio.channels.ScatteringByteChannel;
+public class DoubleByteBuf extends CompositeByteBuf {
 
 Review comment:
   we might need to add an updated javadoc comment back @merlimat  


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 #1099: Fix auditor shutdown logic and move decommision tests out of BookKeeperAdminTest

[GitBox]

sijie commented on a change in pull request #1099: Fix auditor shutdown logic 
and move decommision tests out of BookKeeperAdminTest
URL: https://github.com/apache/bookkeeper/pull/1099#discussion_r165578640
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
 ##
 @@ -707,8 +707,7 @@ public void processResult(int rc, String s, Object obj) {
  */
 public void shutdown() {
 LOG.info("Shutting down auditor");
-submitShutdownTask();
-
+executor.shutdown();
 
 Review comment:
   I explained this in the description:
   
   "the auditor shutdown logic is problematic. most of the tests can finish 
quickly however it spend more 30 seconds on shutting down.
   because the shutdown logic will be blocked until awaitTermination timed 
out." 
   
   if Auditor is running, it might be blocking in the thread, submit shutdown 
task will never be executed, then it will have to wait for 30 seconds timeout 
(`awaitTermination`) and issue `shutdownNow`. this change issues shutdown, 
which will interrupt the auditor task and shutdown the executor thread. 
   
   Hope this make things clear


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 #1111: Update PR template

[GitBox]

sijie opened a new pull request #: Update PR template
URL: https://github.com/apache/bookkeeper/pull/
 
 
   
   Descriptions of the changes in this PR:
   
   - JIRA is not used anymore. Remove it from PR template.
   


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 #1112: Update release schedule

[GitBox]

sijie opened a new pull request #1112: Update release schedule
URL: https://github.com/apache/bookkeeper/pull/1112
 
 
   Descriptions of the changes in this PR:
   
   - update current feature release to 4.7.0 and its release window
   - add a section in release guide for instructions to update release schedule


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 #1113: BP-28: Etcd as metadata store

[GitBox]

sijie opened a new pull request #1113: BP-28: Etcd as metadata store
URL: https://github.com/apache/bookkeeper/pull/1113
 
 
   Descriptions of the changes in this PR:
   
   This BP is exploring using Etcd as the metadata store.


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 #1114: BP-28 (prototype): use Etcd as metadata store

[GitBox]

sijie opened a new pull request #1114: BP-28 (prototype): use Etcd as metadata 
store
URL: https://github.com/apache/bookkeeper/pull/1114
 
 
   Descriptions of the changes in this PR:
   
   This is a prototype change for demonstrating how to implement the metadata 
interfaces using Etcd api.
   
   *BE AWARE*
   
   This is only for prototype purpose. The initial change was done one year and 
half ago and re-picked to current bookkeeper repo. As @jvrao requested, pushed 
it out to raise the discussion.


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 #1114: BP-28 (prototype): use Etcd as metadata store

[GitBox]

sijie commented on issue #1114: BP-28 (prototype): use Etcd as metadata store
URL: https://github.com/apache/bookkeeper/pull/1114#issuecomment-362562660
 
 
   @jvrao as we discussed today, I pushed my prototype changes here and raised 
a BP for discussion.


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 #1113: BP-28: Etcd as metadata store

[GitBox]

sijie commented on issue #1113: BP-28: Etcd as metadata store
URL: https://github.com/apache/bookkeeper/pull/1113#issuecomment-362567530
 
 
   @jvrao : 
https://github.com/sijie/bookkeeper/blob/19590b58a0aa54c9714b339ffedc2ded13ec4160/site/bps/BP-28-etcd-as-metadata-store.md


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

Jenkins build is back to normal : bookkeeper_release_branch #47

[Apache Jenkins Server]

See 

Jenkins build is back to stable : bookkeeper_postcommit_master_java9 #30

[Apache Jenkins Server]

See 

[GitHub] eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165681334
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LastAddSyncedManager.java
 ##
 @@ -0,0 +1,76 @@
+/*
+ *
+ * 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;
+
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Handles LastAddSynced on the client side.
+ */
+class LastAddSyncedManager {
 
 Review comment:
   done


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165681356
 
 

 ##
 File path: .gitignore
 ##
 @@ -20,3 +20,4 @@ lib/
 log/
 target/
 dependency-reduced-pom.xml
+/bookkeeper-server/nbproject/
 
 Review comment:
   done


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165681674
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##
 @@ -1444,17 +1460,23 @@ void sendAddSuccessCallbacks() {
 return;
 }
 // Check if it is the next entry in the sequence.
-if (pendingAddOp.entryId != 0 && pendingAddOp.entryId != 
lastAddConfirmed + 1) {
+if (pendingAddOp.entryId != 0 && pendingAddOp.entryId != 
pendingAddsSequenceHead) {
 
 Review comment:
   as we have pushed server-side changes I will keep *pendingAddsSequenceHead*


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165681963
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##
 @@ -1444,17 +1460,23 @@ void sendAddSuccessCallbacks() {
 return;
 }
 // Check if it is the next entry in the sequence.
-if (pendingAddOp.entryId != 0 && pendingAddOp.entryId != 
lastAddConfirmed + 1) {
+if (pendingAddOp.entryId != 0 && pendingAddOp.entryId != 
pendingAddsSequenceHead) {
 if (LOG.isDebugEnabled()) {
-LOG.debug("Head of the queue entryId: {} is not lac: {} + 
1", pendingAddOp.entryId,
-lastAddConfirmed);
+LOG.debug("Head of the queue entryId: {} is not the 
expected value: {}", pendingAddOp.entryId,
+   pendingAddsSequenceHead);
 }
 return;
 }
 
 pendingAddOps.remove();
 explicitLacFlushPolicy.updatePiggyBackedLac(lastAddConfirmed);
-lastAddConfirmed = pendingAddOp.entryId;
+if (writeFlags.contains(WriteFlag.DEFERRED_SYNC)) {
+this.lastAddConfirmed = 
lastAddSyncedManager.calculateCurrentLastAddSynced();
 
 Review comment:
   done


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165682170
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/DeferredSyncClientSideTest.java
 ##
 @@ -0,0 +1,58 @@
+/*
+ * 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;
+
+import static org.apache.bookkeeper.common.concurrent.FutureUtils.result;
+import static org.junit.Assert.assertEquals;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import org.apache.bookkeeper.client.api.WriteFlag;
+import org.apache.bookkeeper.client.api.WriteHandle;
+import org.junit.Test;
+
+/**
+ * Client side tests on deferred sync write flag.
+ */
+public class DeferredSyncClientSideTest extends MockBookKeeperTestCase {
 
 Review comment:
   done


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165683039
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java
 ##
 @@ -121,7 +121,7 @@ void sendWriteRequest(int bookieIndex) {
 int flags = isRecoveryAdd ? BookieProtocol.FLAG_RECOVERY_ADD : 
BookieProtocol.FLAG_NONE;
 
 
lh.bk.getBookieClient().addEntry(lh.metadata.currentEnsemble.get(bookieIndex), 
lh.ledgerId, lh.ledgerKey,
-entryId, toSend, this, bookieIndex, flags);
+entryId, toSend, this, bookieIndex, flags, lh.writeFlags);
 
 Review comment:
   done


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165689136
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
 ##
 @@ -524,10 +526,15 @@ void writeLac(final long ledgerId, final byte[] 
masterKey, final long lac, ByteB
  *  Add options
  */
 void addEntry(final long ledgerId, byte[] masterKey, final long entryId, 
ByteBuf toSend, WriteCallback cb,
-  Object ctx, final int options) {
+  Object ctx, final int options, final EnumSet 
writeFlags) {
 Object request = null;
 CompletionKey completionKey = null;
 if (useV2WireProtocol) {
+if (writeFlags.contains(WriteFlag.DEFERRED_SYNC)) {
+LOG.error("invalid writeflags {} for v2 protocol", writeFlags);
+toSend.release();
+cb.writeComplete(BKException.Code.IllegalOpException, 
ledgerId, entryId, addr, ctx);
 
 Review comment:
   done


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on issue #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client 
Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#issuecomment-362631969
 
 
   @sijie I have addressed all of your review comments, except from dropping 
*pendingAddsSequenceHead*,  because now we have server side implementation.
   
   Do you think we should add BC tests in this commit ? or it is better to have 
a new commit/pr ? now each new BC tests requires a new maven module


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on a change in pull request #853: WIP - BP-14 WriteFlag 
DEFERRED_SYNC Client Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#discussion_r165689136
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
 ##
 @@ -524,10 +526,15 @@ void writeLac(final long ledgerId, final byte[] 
masterKey, final long lac, ByteB
  *  Add options
  */
 void addEntry(final long ledgerId, byte[] masterKey, final long entryId, 
ByteBuf toSend, WriteCallback cb,
-  Object ctx, final int options) {
+  Object ctx, final int options, final EnumSet 
writeFlags) {
 Object request = null;
 CompletionKey completionKey = null;
 if (useV2WireProtocol) {
+if (writeFlags.contains(WriteFlag.DEFERRED_SYNC)) {
+LOG.error("invalid writeflags {} for v2 protocol", writeFlags);
+toSend.release();
+cb.writeComplete(BKException.Code.IllegalOpException, 
ledgerId, entryId, addr, ctx);
 
 Review comment:
   done, see BookKeeperTest#testCannotUseWriteFlagsOnV2Protocol


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] reddycharan commented on a change in pull request #1099: Fix auditor shutdown logic and move decommision tests out of BookKeeperAdminTest

[GitBox]

reddycharan commented on a change in pull request #1099: Fix auditor shutdown 
logic and move decommision tests out of BookKeeperAdminTest
URL: https://github.com/apache/bookkeeper/pull/1099#discussion_r165711029
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
 ##
 @@ -707,8 +707,7 @@ public void processResult(int rc, String s, Object obj) {
  */
 public void shutdown() {
 LOG.info("Shutting down auditor");
-submitShutdownTask();
-
+executor.shutdown();
 
 Review comment:
   yeah I get that, but I'm wondering why it wasn't like this before? why did 
the original author - 
https://github.com/apache/bookkeeper/commit/e5b0dd0d9e47394202f5a20cedf409f487d90beb
 went through this extra round of hassle for shutdown deliberately. Is it ok 
now to not to have this extra scrutiny with the shutdown process?


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

sijie commented on issue #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side 
Implementation
URL: https://github.com/apache/bookkeeper/pull/853#issuecomment-362654400
 
 
   @eolivelli is this still "WIP" or ready for review?


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165711899
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -1430,19 +1432,44 @@ public void triggerAudit()
 public void decommissionBookie(BookieSocketAddress bookieAddress)
 throws CompatibilityException, UnavailableException, 
KeeperException, InterruptedException, IOException,
 BKAuditException, TimeoutException, BKException {
+int maxSleepTimeInBetweenChecks = 10 * 60 * 1000; // 10 minutes
+int sleepTimePerLedger = 10 * 1000; // 10 secs
+
+decommissionBookie(
+bookieAddress,
+// default wait strategy is to sleep for 30 secs.
+// so that Auditor gets chance to trigger its
+// force audittask and let the underreplicationmanager process
+// to do its replication process
+() -> {
+Thread.sleep(30 * 1000);
+return null;
+},
+sleepTimePerLedger,
+maxSleepTimeInBetweenChecks);
+}
+
+@VisibleForTesting
+public void decommissionBookie(BookieSocketAddress bookieAddress,
+   Callable waitAuditorStrategy,
+   int sleepTimePerLedger,
+   int maxSleepTimeInBetweenChecks)
+throws CompatibilityException, UnavailableException, 
KeeperException, InterruptedException, IOException,
+BKAuditException, TimeoutException, BKException {
 if (getAvailableBookies().contains(bookieAddress) || 
getReadOnlyBookies().contains(bookieAddress)) {
 LOG.error("Bookie: {} is not shutdown yet", bookieAddress);
 throw BKException.create(BKException.Code.IllegalOpException);
 }
 
 triggerAudit();
 
-/*
- * Sleep for 30 secs, so that Auditor gets chance to trigger its
- * force audittask and let the underreplicationmanager process
- * to do its replication process
- */
-Thread.sleep(30 * 1000);
+try {
+waitAuditorStrategy.call();
+} catch (InterruptedException ie) {
+throw ie;
+} catch (Exception e) {
+throw new TimeoutException("Encountered exception on waiting 
auditor auditing the bookies");
 
 Review comment:
   not sure if the TimeoutException is the appropriate one in this case.


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 #1099: Fix auditor shutdown logic and move decommision tests out of BookKeeperAdminTest

[GitBox]

sijie commented on a change in pull request #1099: Fix auditor shutdown logic 
and move decommision tests out of BookKeeperAdminTest
URL: https://github.com/apache/bookkeeper/pull/1099#discussion_r165718676
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
 ##
 @@ -707,8 +707,7 @@ public void processResult(int rc, String s, Object obj) {
  */
 public void shutdown() {
 LOG.info("Shutting down auditor");
-submitShutdownTask();
-
+executor.shutdown();
 
 Review comment:
   No idea why it was done in that way. @ivankelly can chime in since he made 
that change.


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165720124
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -1430,19 +1432,44 @@ public void triggerAudit()
 public void decommissionBookie(BookieSocketAddress bookieAddress)
 throws CompatibilityException, UnavailableException, 
KeeperException, InterruptedException, IOException,
 BKAuditException, TimeoutException, BKException {
+int maxSleepTimeInBetweenChecks = 10 * 60 * 1000; // 10 minutes
+int sleepTimePerLedger = 10 * 1000; // 10 secs
+
+decommissionBookie(
+bookieAddress,
+// default wait strategy is to sleep for 30 secs.
+// so that Auditor gets chance to trigger its
+// force audittask and let the underreplicationmanager process
+// to do its replication process
+() -> {
+Thread.sleep(30 * 1000);
+return null;
+},
+sleepTimePerLedger,
+maxSleepTimeInBetweenChecks);
+}
+
+@VisibleForTesting
+public void decommissionBookie(BookieSocketAddress bookieAddress,
+   Callable waitAuditorStrategy,
+   int sleepTimePerLedger,
+   int maxSleepTimeInBetweenChecks)
+throws CompatibilityException, UnavailableException, 
KeeperException, InterruptedException, IOException,
+BKAuditException, TimeoutException, BKException {
 if (getAvailableBookies().contains(bookieAddress) || 
getReadOnlyBookies().contains(bookieAddress)) {
 LOG.error("Bookie: {} is not shutdown yet", bookieAddress);
 throw BKException.create(BKException.Code.IllegalOpException);
 }
 
 triggerAudit();
 
-/*
- * Sleep for 30 secs, so that Auditor gets chance to trigger its
- * force audittask and let the underreplicationmanager process
- * to do its replication process
- */
-Thread.sleep(30 * 1000);
+try {
+waitAuditorStrategy.call();
+} catch (InterruptedException ie) {
+throw ie;
+} catch (Exception e) {
+throw new TimeoutException("Encountered exception on waiting 
auditor auditing the bookies");
 
 Review comment:
   okay any better exceptions?


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165722458
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
 
 Review comment:
   why do we have this if block? in which case it can be null and is it ok to 
be null for our testscenario?


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165725190
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
+setAutoRecoveryEnabled(false);
 
 Review comment:
   I'm not sure how can this be ok? you disabled AutoRecoveryService for all 
the bookies by setting this to false, but in each testcase you are starting  
AutoRecoveryMain (which starts daemon for Auditor and ReplicationWorker) just 
for one bookie. How come having just one bookie ReplicationWorker would be 
sufficient for these testcases where we are killing bookie/s? 
   
   My concern here is if the bookie which is having AutoRecoveryMain is already 
having copy of fragments of that ledger, then it would ignore replicating that 
under replicated ledger to that Bookie, right?


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165725190
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
+setAutoRecoveryEnabled(false);
 
 Review comment:
   I'm not sure how can this be ok? you disabled AutoRecoveryService for all 
the bookies by setting this to false, but in each testcase you are starting  
AutoRecoveryMain (which starts daemon for Auditor and ReplicationWorker) just 
for one bookie. How come having just one bookie ReplicationWorker would be 
sufficient for these testcases where we are killing bookie/s? 
   
   My concern here is if the bookie which is having AutoRecoveryMain is already 
having copy of fragments of that ledger, then it would ignore replicating that 
under replicated ledger to this Bookie, right?


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165726882
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
+auditor.submitLostBookieRecoveryDelayChangedEvent().get();
+log.info("Wait until auditor completes the auditing 
task.");
 
 Review comment:
   isnt this log line supposed to be before the call 


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165728368
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
 
 Review comment:
   same comment as above


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165728252
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperAdminTest.java
 ##
 @@ -61,7 +61,7 @@
 public BookKeeperAdminTest() {
 super(numOfBookies, 480);
 baseConf.setLostBookieRecoveryDelay(lostBookieRecoveryDelayInitValue);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
+baseConf.setOpenLedgerRereplicationGracePeriod(500);
 
 Review comment:
   is this change required? does it affect anyhow? for the sake of simplicity 
cannt we just leave it to default value


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165732674
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
 
 Review comment:
   this is just for safety guard


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165733652
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
+auditor.submitLostBookieRecoveryDelayChangedEvent().get();
+log.info("Wait until auditor completes the auditing 
task.");
 
 Review comment:
   will update to a more meaningful sentence. 


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165733498
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
+setAutoRecoveryEnabled(false);
 
 Review comment:
   the way I see this - what decommission test is doing here is to testing the 
decommission behavior. we are not testing killing auto recovery. we don't need 
to start autorecovery on every bookie, it is resource consumed and don't have 
more meanings to the test itself. move auto recovery process out of the 
bookies, it can make the auto recovery trigger more deterministic, rather than 
replying on non-deterministic sleeping 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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165733931
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperAdminTest.java
 ##
 @@ -61,7 +61,7 @@
 public BookKeeperAdminTest() {
 super(numOfBookies, 480);
 baseConf.setLostBookieRecoveryDelay(lostBookieRecoveryDelayInitValue);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
+baseConf.setOpenLedgerRereplicationGracePeriod(500);
 
 Review comment:
   well the point of having this change is to reduce the test time on this test 
case. 30 seconds is too much for a test... why do we need want to test 30 
seconds...


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165734169
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
 
 Review comment:
   same as above. the point of this change is to reduce the test time. we 
unnecessarily have a lot of such 30 seconds sleep in quite a few test cases. I 
would like to kill them or reduce them to a small value.


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 #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client Side Implementation

[GitBox]

eolivelli commented on issue #853: WIP - BP-14 WriteFlag DEFERRED_SYNC Client 
Side Implementation
URL: https://github.com/apache/bookkeeper/pull/853#issuecomment-362680500
 
 
   It is ready


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 #1115: Change default ledger manager factory from `Flat` to `Hierachical`

[GitBox]

sijie opened a new pull request #1115: Change default ledger manager factory 
from `Flat` to `Hierachical`
URL: https://github.com/apache/bookkeeper/pull/1115
 
 
   Descriptions of the changes in this PR:
   
   the discussion can be found at : 
http://mail-archives.apache.org/mod_mbox/bookkeeper-dev/201802.mbox/%3CCAO2yDyYdZ8%3D4tViNX1uL0Z67KX78JPD2XBGd7ermjO%3DK%3DscvcQ%40mail.gmail.com%3E


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 #1116: Cleanup registration client interface

[GitBox]

sijie opened a new pull request #1116: Cleanup registration client interface
URL: https://github.com/apache/bookkeeper/pull/1116
 
 
   Descriptions of the changes in this PR:
   
   This change is mainly to remove `zookeeper` reference from metadata 
interface. The existence of `Optional` is to allow passing an 
external zookeeper client to bookkeeper, so bookkeeper can reuse that client 
instance. That is useful for services like pulsar broker, which they can only 
instantiate a zookeeper client and pass the zookeeper client around to 
construct bookkeeper client. However, this pollutes the registration client 
interface, change the method to `Optional` as an optional context 
object and let the implementation interpret 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 issue #1116: Cleanup registration client interface

[GitBox]

sijie commented on issue #1116: Cleanup registration client interface
URL: https://github.com/apache/bookkeeper/pull/1116#issuecomment-362693360
 
 
   @jiazhai since you did the interface abstraction, can you take a look to see 
if this work for you.
   
   @merlimat since pulsar is the main user of external zookeeper instance, can 
you review to make sure it is okay from your perspective.
   
   to be clear on this change, it would be ideal to remove that field 
completely. however since bookkeeper still supports passing external zookeeper 
client and most likely it won't be gone for any time soon, so I stick to just 
change it to `Optional` to remove zookeeper reference from metadata 
interfaces entirely. Let the implementation itself interpret this context 
object.


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 #1115: Change default ledger manager factory from `Flat` to `Hierachical`

[GitBox]

sijie commented on issue #1115: Change default ledger manager factory from 
`Flat` to `Hierachical`
URL: https://github.com/apache/bookkeeper/pull/1115#issuecomment-362693556
 
 
   > Maybe we can add @deprecated annotations to FlatLedgerManagerFactory class?
   
   good idea


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 #1115: Change default ledger manager factory from `Flat` to `Hierachical`

[GitBox]

sijie commented on issue #1115: Change default ledger manager factory from 
`Flat` to `Hierachical`
URL: https://github.com/apache/bookkeeper/pull/1115#issuecomment-362698987
 
 
   @yzang : make `FlatLedgerManagerFactory` as `@deprecated`


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165753916
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
+auditor.submitLostBookieRecoveryDelayChangedEvent().get();
+log.info("Wait until auditor completes the auditing 
task.");
 
 Review comment:
   no I mean, isn't the log line supposed to be before 
auditor.submitLostBookieRecoveryDelayChangedEvent() call?
   
   log.info("Wait until auditor completes the auditing task.");
   auditor.submitLostBookieRecoveryDelayChangedEvent().get();
   


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165754551
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
 
 Review comment:
   hmm..in hypothetical scenario if it is null, then aren't we just ignoring 
it? is that ok to just ignore the fact that auditor is null and proceed with 
the test logic without waiting for auditor to do its auditing bookies task? 
   
   Anyhow, I'm still not convinced how can we be ok with Auditor to be null? If 
there is no Auditor, then there is no replication process, right?


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165756792
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
+setAutoRecoveryEnabled(false);
 
 Review comment:
   I dont get your point  - "we don't need to start autorecovery on every 
bookie"
   
   lets say there are 6 Bookies - BK0, BK1, BK2,..BK5 and Ledger with quorum 
3,3 is created.
   you started AutoRecovery process for only one Bookie - BK0. So for BK0 
Auditor and ReplicationWorker daemons are running.
   
   LID-P (ledger) : BK0, BK1, BK2 (ensemble)
   
   Now if BK1 is killed, so LID-P will be underreplicated. My question is 
though AutoRecovery (Auditor, ReplicationWorker) are running for BK0, LID-P 
will remain under-replicated, right? Since, BK0 is already part of ensemble of 
LID-P, ReplicationWorker running with BK0 wouldn't be able to do anything.


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165757994
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperAdminTest.java
 ##
 @@ -61,7 +61,7 @@
 public BookKeeperAdminTest() {
 super(numOfBookies, 480);
 baseConf.setLostBookieRecoveryDelay(lostBookieRecoveryDelayInitValue);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
+baseConf.setOpenLedgerRereplicationGracePeriod(500);
 
 Review comment:
   ok..i forgot that in this test 
testDecommissionForLedgersWithMultipleSegmentsAndNotWriteClosed, there are not 
writeclosed ledgers and they will remain underreplicated for 
openLedgerRereplicationGracePeriod.


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165758468
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
 
 Review comment:
   ok..i forgot that in this test 
testDecommissionForLedgersWithMultipleSegmentsAndNotWriteClosed, there are not 
writeclosed ledgers and they will remain underreplicated for 
openLedgerRereplicationGracePeriod. 
   
   Anyhow isn't 100 millsec too less? 


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165759553
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -1430,19 +1432,44 @@ public void triggerAudit()
 public void decommissionBookie(BookieSocketAddress bookieAddress)
 throws CompatibilityException, UnavailableException, 
KeeperException, InterruptedException, IOException,
 BKAuditException, TimeoutException, BKException {
+int maxSleepTimeInBetweenChecks = 10 * 60 * 1000; // 10 minutes
+int sleepTimePerLedger = 10 * 1000; // 10 secs
+
+decommissionBookie(
+bookieAddress,
+// default wait strategy is to sleep for 30 secs.
+// so that Auditor gets chance to trigger its
+// force audittask and let the underreplicationmanager process
+// to do its replication process
+() -> {
+Thread.sleep(30 * 1000);
+return null;
+},
+sleepTimePerLedger,
+maxSleepTimeInBetweenChecks);
+}
+
+@VisibleForTesting
+public void decommissionBookie(BookieSocketAddress bookieAddress,
+   Callable waitAuditorStrategy,
+   int sleepTimePerLedger,
+   int maxSleepTimeInBetweenChecks)
+throws CompatibilityException, UnavailableException, 
KeeperException, InterruptedException, IOException,
+BKAuditException, TimeoutException, BKException {
 if (getAvailableBookies().contains(bookieAddress) || 
getReadOnlyBookies().contains(bookieAddress)) {
 LOG.error("Bookie: {} is not shutdown yet", bookieAddress);
 throw BKException.create(BKException.Code.IllegalOpException);
 }
 
 triggerAudit();
 
-/*
- * Sleep for 30 secs, so that Auditor gets chance to trigger its
- * force audittask and let the underreplicationmanager process
- * to do its replication process
- */
-Thread.sleep(30 * 1000);
+try {
+waitAuditorStrategy.call();
+} catch (InterruptedException ie) {
+throw ie;
+} catch (Exception e) {
+throw new TimeoutException("Encountered exception on waiting 
auditor auditing the bookies");
 
 Review comment:
   BKAuditException? even in the InterruptedException case also. 
   
   This pattern is used in other places also
   
   
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L692


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165754551
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
 
 Review comment:
   hmm..in hypothetical scenario if it is null, then aren't we just ignoring 
it? is that ok to just ignore the fact that auditor is null and proceed with 
the decommission logic without waiting for auditor to do its auditing bookies 
task? 
   
   Anyhow, I'm still not convinced how can we be ok with Auditor to be null? If 
there is no Auditor, then there is no replication process, right?


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165763335
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
 
 Review comment:
   well I think the test is to wait until the open fragments are fenced (unless 
I missed something). so if the test is to wait until open fragments are fenced 
and replicated, then 100ms isn't too less.


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165763534
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
+auditor.submitLostBookieRecoveryDelayChangedEvent().get();
+log.info("Wait until auditor completes the auditing 
task.");
 
 Review comment:
   I mean my intention to add logging after that, so I will update the sentence 
to a more meaningful sentence.


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165764523
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
+setAutoRecoveryEnabled(false);
 
 Review comment:
   @reddycharan I think you have a wrong understanding on how autorecovery 
works. autorecovery is a completely isolated thing from bookies, it doesn't 
really stick to any bookies. It can be co-run with bookies, or it can be run as 
separate service. that means you can run less or more autorecovery instances 
that bookies.
   
   in your example, if BK1 is killed, autorecovery daemon will detect that and 
choose a bookie to replace BK1 and replicate entries from BK0 and BK2 to the 
newly chosen bookie.


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 #1100: Improve decomission test

[GitBox]

sijie commented on a change in pull request #1100: Improve decomission test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165764894
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
 
 Review comment:
   > If there is no Auditor, then there is no replication process, right?
   
   If there is no auditor, there is no replication process then the test will 
fail.
   
   The logic here is just to avoid throw NPE (it is a coding style problem). I 
can change it to checkNotNull(auditor) to explicitly throw NPE if you like.


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165777075
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -44,14 +46,17 @@
 
 public BookieDecommissionTest() {
 super(NUM_OF_BOOKIES, 480);
-baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
-setAutoRecoveryEnabled(true);
+baseConf.setOpenLedgerRereplicationGracePeriod(100);
+setAutoRecoveryEnabled(false);
 
 Review comment:
   ok..had private chat with Sijie. 
https://github.com/apache/bookkeeper/commit/6fcabfc80c5bd6cdfa9252cdfc4ed209cc0620a6
 change has decoupled ReplicationWorker and Bookie completely, and now there is 
no "targetBookie" factor in ReplicationWorker. So I'm good with this change. On 
a different note, ReplicationWorker.java javadoc is not updated with that 
change and it is still using "targetBookie" in its comments.


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] reddycharan commented on a change in pull request #1100: Improve decomission test

[GitBox]

reddycharan commented on a change in pull request #1100: Improve decomission 
test
URL: https://github.com/apache/bookkeeper/pull/1100#discussion_r165777903
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -86,7 +91,18 @@ public void testDecommissionBookie() throws Exception {
  * this decommisionBookie should make sure that there are no
  * underreplicated ledgers because of this bookie
  */
-bkAdmin.decommissionBookie(Bookie.getBookieAddress(killedBookieConf));
+bkAdmin.decommissionBookie(
+Bookie.getBookieAddress(killedBookieConf),
+() -> {
+Auditor auditor = recoveryProcess.getAuditor();
+if (null != auditor) {
 
 Review comment:
   since recoveryProcess.start() is async call here, as we discuss wait until 
auditor is not null 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] eolivelli commented on a change in pull request #1115: Change default ledger manager factory from `Flat` to `Hierachical`

[GitBox]

eolivelli commented on a change in pull request #1115: Change default ledger 
manager factory from `Flat` to `Hierachical`
URL: https://github.com/apache/bookkeeper/pull/1115#discussion_r165779742
 
 

 ##
 File path: deploy/kubernetes/gke/bookkeeper.statefulset.yml
 ##
 @@ -39,8 +39,6 @@ data:
 BK_ledgerDirectories: "/bookkeeper/data/ledgers"
 BK_indexDirectories: "/bookkeeper/data/ledgers"
 BK_zkServers: zookeeper
-# the default manager is flat, which is not good for supporting large 
number of ledgers
-BK_ledgerManagerType: "hierarchical"
 
 Review comment:
   We could leave this line so that it is explicit this important configuration 
parameter


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 #1115: Change default ledger manager factory from `Flat` to `Hierachical`

[GitBox]

eolivelli commented on a change in pull request #1115: Change default ledger 
manager factory from `Flat` to `Hierachical`
URL: https://github.com/apache/bookkeeper/pull/1115#discussion_r165779555
 
 

 ##
 File path: deploy/kubernetes/gke/bookkeeper.yaml
 ##
 @@ -30,8 +30,6 @@ data:
 BK_ledgerDirectories: "/bookkeeper/data/ledgers"
 BK_indexDirectories: "/bookkeeper/data/ledgers"
 BK_zkServers: zookeeper
-# the default manager is flat, which is not good for supporting large 
number of ledgers
-BK_ledgerManagerType: "hierarchical"
 
 Review comment:
   We could leave this line so that it is explicit this important configuration 
parameter


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 #1108: Replace DoubleByteBuf with CompositeByteBuf because of perf regression with Netty > 4.1.12

[GitBox]

eolivelli commented on issue #1108: Replace DoubleByteBuf with CompositeByteBuf 
because of perf regression with Netty > 4.1.12
URL: https://github.com/apache/bookkeeper/pull/1108#issuecomment-362730853
 
 
   Please fix checkstyle, which wants the java doc comment


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 #1110: Issue 1109: Error out pending ops on TLS key mismatch exception

[GitBox]

eolivelli commented on a change in pull request #1110: Issue 1109: Error out 
pending ops on TLS key mismatch exception
URL: https://github.com/apache/bookkeeper/pull/1110#discussion_r165780957
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
 ##
 @@ -230,7 +232,43 @@ public void testStartTLSServerNoKeyStore() throws 
Exception {
 }
 
 /**
- * Verify that a server will not start if tls is enabled but the cert 
password is incorrect.
+ * Verify handshake failure with a bad cert.
+ */
+@Test(timeout = 6)
 
 Review comment:
   Nit: drop timeout


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 #1110: Issue 1109: Error out pending ops on TLS key mismatch exception

[GitBox]

eolivelli commented on a change in pull request #1110: Issue 1109: Error out 
pending ops on TLS key mismatch exception
URL: https://github.com/apache/bookkeeper/pull/1110#discussion_r165780705
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
 ##
 @@ -1014,9 +1032,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, 
Throwable cause) throws E
 }
 
 if (cause instanceof IOException) {
-// these are thrown when a bookie fails, logging them just pollutes
 
 Review comment:
   Any strong reason for dropping the comment and adding this log?


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 #1115: Change default ledger manager factory from `Flat` to `Hierachical`

[GitBox]

sijie commented on a change in pull request #1115: Change default ledger 
manager factory from `Flat` to `Hierachical`
URL: https://github.com/apache/bookkeeper/pull/1115#discussion_r165787436
 
 

 ##
 File path: deploy/kubernetes/gke/bookkeeper.statefulset.yml
 ##
 @@ -39,8 +39,6 @@ data:
 BK_ledgerDirectories: "/bookkeeper/data/ledgers"
 BK_indexDirectories: "/bookkeeper/data/ledgers"
 BK_zkServers: zookeeper
-# the default manager is flat, which is not good for supporting large 
number of ledgers
-BK_ledgerManagerType: "hierarchical"
 
 Review comment:
   `ledgerMangerType` is already deprecated long time ago. it doesn't make any 
sense to leave it here. we used the wrong parameter here so I don't see a 
reason to have it though. for people who is new to bookkeeper, he should be 
knowing less settings as possible.


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 #1117: BP-29: Metadata API module

[GitBox]

sijie opened a new pull request #1117: BP-29: Metadata API module
URL: https://github.com/apache/bookkeeper/pull/1117
 
 
   Descriptions of the changes in this PR:
   
   Related to BP-28 (#1113), this proposal is to propose how we want to 
organize the metadata modules, to support different metadata storage 
implementation.


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 #1117: BP-29: Metadata API module

[GitBox]

sijie commented on issue #1117: BP-29: Metadata API module
URL: https://github.com/apache/bookkeeper/pull/1117#issuecomment-362744290
 
 
   @jvrao as discussed, here is the proposal : 
https://github.com/sijie/bookkeeper/blob/c9b47373754f78cadb325ffd4b7a6b962940a12e/site/bps/BP-29-metadata-store-api-module.md
   
   How the module would looks like: 
https://github.com/sijie/bookkeeper/tree/metadata-store-api-module/metadata-stores


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 #1063: PendingAddOp keeps refCnt onthe toSend buffer longer than needed.

[GitBox]

sijie closed issue #1063: PendingAddOp keeps refCnt onthe toSend buffer longer 
than needed.
URL: https://github.com/apache/bookkeeper/issues/1063
 
 
   


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 #1091: Issue 1063: Write keeps refCnt longer

[GitBox]

sijie commented on issue #1091: Issue 1063: Write keeps refCnt longer
URL: https://github.com/apache/bookkeeper/pull/1091#issuecomment-362750080
 
 
   thanks @jvrao . merge it and also cherry-picked to branch-4.6, since it is a 
good fix to be picked at 4.6.


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 #1091: Issue 1063: Write keeps refCnt longer

[GitBox]

sijie closed pull request #1091: Issue 1063: Write keeps refCnt longer
URL: https://github.com/apache/bookkeeper/pull/1091
 
 
   

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-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java
index 77f05c15a..e386701d7 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java
@@ -383,29 +383,40 @@ private PendingAddOp(Handle recyclerHandle) 
{
 this.recyclerHandle = recyclerHandle;
 }
 
+
 private void maybeRecycle() {
-// The reference to PendingAddOp can be held in 3 places
-// - LedgerHandle#pendingAddOp
-//   This reference is released when the callback is run
-// - The executor
-//   Released after safeRun finishes
-// - BookieClient
-//   Holds a reference from the point the addEntry requests are
-//   sent.
-// The object can only be recycled after all references are
-// released, otherwise we could end up recycling twice and all
-// joy that goes along with that.
-if (hasRun && callbackTriggered && pendingWriteRequests == 0) {
-recycle();
+/**
+ * We have opportunity to recycle two objects here.
+ * PendingAddOp#toSend and LedgerHandle#pendingAddOp
+ *
+ * A. LedgerHandle#pendingAddOp: This can be released after all 3 
conditions are met.
+ *- After the callback is run
+ *- After safeRun finished by the executor
+ *- Write responses are returned from all bookies
+ *  as BookieClient Holds a reference from the point the addEntry 
requests are sent.
+ *
+ * B. ByteBuf can be released after 2 of the conditions are met.
+ *- After the callback is run as we will not retry the write after 
callback
+ *- After safeRun finished by the executor
+ * BookieClient takes and releases on this buffer immediately after 
sending the data.
+ *
+ * The object can only be recycled after the above conditions are met
+ * otherwise we could end up recycling twice and all
+ * joy that goes along with that.
+ */
+if (hasRun && callbackTriggered) {
+ReferenceCountUtil.release(toSend);
+toSend = null;
+}
+if (toSend == null && pendingWriteRequests == 0) {
+recyclePendAddOpObject();
 }
 }
 
-private void recycle() {
+private void recyclePendAddOpObject() {
 entryId = LedgerHandle.INVALID_ENTRY_ID;
 currentLedgerLength = -1;
-ReferenceCountUtil.release(toSend);
 payload = null;
-toSend = null;
 cb = null;
 ctx = null;
 ackSet.recycle();


 


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 #970: ISSUE #966: Expose quorum write complete latency to the client

[GitBox]

sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency 
to the client
URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-362750216
 
 
   @eolivelli can you review this again? @athanatos addressed your comment.


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] reddycharan commented on issue #1097: Use loopback network interface for testcases.

[GitBox]

reddycharan commented on issue #1097: Use loopback network interface for 
testcases.
URL: https://github.com/apache/bookkeeper/pull/1097#issuecomment-362750431
 
 
   I ran locally AuditorLedgerCheckerTest and BookieClientTest multiple times 
and they are running fine. Not sure why tests failed here. Also different tests 
failed in Java8 and Java9 build.


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] reddycharan commented on issue #1097: Use loopback network interface for testcases.

[GitBox]

reddycharan commented on issue #1097: Use loopback network interface for 
testcases.
URL: https://github.com/apache/bookkeeper/pull/1097#issuecomment-362750431
 
 
   I ran locally AuditorLedgerCheckerTest and BookieClientTest multiple times 
and they are running fine. Not sure why tests failed here. Also different tests 
in Java8 and Java9 build.


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 #1067: RecoveryReadOp: a single hanging bookie can prevent a ledger from fencing

[GitBox]

sijie closed issue #1067: RecoveryReadOp: a single hanging bookie can prevent a 
ledger from fencing
URL: https://github.com/apache/bookkeeper/issues/1067
 
 
   


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 #1077: ISSUE #1067: PendingReadOp: recovery, return NoSuchEntry on wQ-aQ+1 errors

[GitBox]

sijie closed pull request #1077: ISSUE #1067: PendingReadOp: recovery, return 
NoSuchEntry on wQ-aQ+1 errors
URL: https://github.com/apache/bookkeeper/pull/1077
 
 
   

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-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java
index 041ccd1d2..58ee31dcd 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java
@@ -74,7 +74,7 @@
 OpStatsLogger readOpLogger;
 private final Counter speculativeReadCounter;
 
-final int maxMissedReadsAllowed;
+final int requiredBookiesMissingEntryForRecovery;
 final boolean isRecoveryRead;
 boolean parallelRead = false;
 final AtomicBoolean complete = new AtomicBoolean(false);
@@ -85,7 +85,7 @@
 
 int rc = BKException.Code.OK;
 int firstError = BKException.Code.OK;
-int numMissedEntryReads = 0;
+int numBookiesMissingEntry = 0;
 
 final ArrayList ensemble;
 final DistributionSchedule.WriteSet writeSet;
@@ -200,7 +200,7 @@ synchronized void logErrorAndReattemptRead(int bookieIndex, 
BookieSocketAddress
 }
 if (BKException.Code.NoSuchEntryException == rc
 || BKException.Code.NoSuchLedgerExistsException == rc) {
-++numMissedEntryReads;
+++numBookiesMissingEntry;
 if (LOG.isDebugEnabled()) {
 LOG.debug("No such entry found on bookie.  L{} E{} bookie: 
{}",
 lh.ledgerId, entryImpl.getEntryId(), host);
@@ -298,14 +298,15 @@ void read() {
 @Override
 synchronized void logErrorAndReattemptRead(int bookieIndex, 
BookieSocketAddress host, String errMsg, int rc) {
 super.logErrorAndReattemptRead(bookieIndex, host, errMsg, rc);
---numPendings;
 // if received all responses or this entry doesn't meet quorum 
write, complete the request.
-if (numMissedEntryReads > maxMissedReadsAllowed || numPendings == 
0) {
-if (BKException.Code.BookieHandleNotAvailableException == 
firstError
-&& numMissedEntryReads > maxMissedReadsAllowed) {
-firstError = BKException.Code.NoSuchEntryException;
-}
 
+--numPendings;
+if (isRecoveryRead && numBookiesMissingEntry >= 
requiredBookiesMissingEntryForRecovery) {
+/* For recovery, report NoSuchEntry as soon as wQ-aQ+1 bookies 
report that they do not
+ * have the entry */
+fail(BKException.Code.NoSuchEntryException);
+} else if (numPendings == 0) {
+// if received all responses, complete the request.
 fail(firstError);
 }
 }
@@ -364,8 +365,9 @@ synchronized BookieSocketAddress 
maybeSendSpeculativeRead(BitSet heardFrom) {
 BitSet sentTo = getSentToBitSet();
 sentTo.and(heardFrom);
 
-// only send another read, if we have had no response at all (even 
for other entries)
-// from any of the other bookies we have sent the request to
+// only send another read if we have had no successful response at 
all
+// (even for other entries) from any of the other bookies we have 
sent the
+// request to
 if (sentTo.cardinality() == 0) {
 speculativeReadCounter.inc();
 return sendNextRead();
@@ -383,14 +385,6 @@ synchronized BookieSocketAddress sendNextRead() {
 if (nextReplicaIndexToReadFrom >= 
getLedgerMetadata().getWriteQuorumSize()) {
 // we are done, the read has failed from all replicas, just 
fail the
 // read
-
-// Do it a bit pessimistically, only when finished trying all 
replicas
-// to check whether we received more missed reads than 
maxMissedReadsAllowed
-if (BKException.Code.BookieHandleNotAvailableException == 
firstError
-&& numMissedEntryReads > maxMissedReadsAllowed) {
-firstError = BKException.Code.NoSuchEntryException;
-}
-
 fail(firstError);
 return null;
 }
@@ -424,6 +418,13 @@ synchronized void logErrorAndReattemptRead(int 
bookieIndex, BookieSocketAddress
 }
 erroredReplicas.set(replica);
 
+if (isRecoveryRead && (numBookiesMissingEntry >= 
requiredBookiesMissingEntryForRecovery)) {
+/* For recovery, report No

[GitHub] reddycharan commented on issue #1097: Use loopback network interface for testcases.

[GitBox]

reddycharan commented on issue #1097: Use loopback network interface for 
testcases.
URL: https://github.com/apache/bookkeeper/pull/1097#issuecomment-362750692
 
 
   @sijie I made changes to the initial commit and replaced the initial commit. 
Can you sign-off on this. Thanks.


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 #1097: Use loopback network interface for testcases.

[GitBox]

sijie commented on issue #1097: Use loopback network interface for testcases.
URL: https://github.com/apache/bookkeeper/pull/1097#issuecomment-362750747
 
 
   @reddycharan I don't think the failures are related.


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 #1095: BP-26 (task 3) : Build dlog shaded jar in bookkeeper way

[GitBox]

sijie closed pull request #1095: BP-26 (task 3) : Build dlog shaded jar in 
bookkeeper way
URL: https://github.com/apache/bookkeeper/pull/1095
 
 
   

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-server/pom.xml b/bookkeeper-server/pom.xml
index f0721b6ab..ba6f24891 100644
--- a/bookkeeper-server/pom.xml
+++ b/bookkeeper-server/pom.xml
@@ -220,46 +220,6 @@
 true
 true
   
-  
-org.apache.maven.plugins
-maven-shade-plugin
-${maven-shade-plugin.version}
-
-  
-
-
-  
-package
-
-  shade
-
-
-  true
-  
false
-  true
-  shaded
-  true
-  true
-  
-
-  com.google.guava:guava
-  com.google.protobuf:protobuf-java
-  org.apache.bookkeeper:bookkeeper-common
-  org.apache.bookkeeper:bookkeeper-proto
-  org.apache.bookkeeper:circe-checksum
-  
org.apache.bookkeeper.stats:bookkeeper-stats-api
-
-  
-  
-
-  com.google
-  
org.apache.bookkeeper.shaded.com.google
-
-  
-
-  
-
-  
   
 org.apache.maven.plugins
 maven-jar-plugin
diff --git a/shaded/distributedlog-core-shaded/pom.xml 
b/shaded/distributedlog-core-shaded/pom.xml
new file mode 100644
index 0..ba147ab5c
--- /dev/null
+++ b/shaded/distributedlog-core-shaded/pom.xml
@@ -0,0 +1,245 @@
+
+
+http://maven.apache.org/POM/4.0.0 
http://maven.apache.org/xsd/maven-4.0.0.xsd"; 
xmlns="http://maven.apache.org/POM/4.0.0";
+xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";>
+  4.0.0
+  
+org.apache.bookkeeper
+shaded-parent
+4.7.0-SNAPSHOT
+..
+  
+  org.apache.distributedlog
+  distributedlog-core-shaded
+  Apache BookKeeper :: Shaded :: distributedlog-core-shaded
+  
+UTF-8
+  
+  
+
+  org.apache.distributedlog
+  distributedlog-core
+  ${project.version}
+  
+
+  org.slf4j
+  slf4j-log4j12
+
+
+  log4j
+  log4j
+
+  
+
+  
+  
+
+  
+org.apache.maven.plugins
+maven-shade-plugin
+${maven-shade-plugin.version}
+
+  
+package
+
+  shade
+
+
+  true
+  
true
+  false
+  
+
+  commons-codec:commons-codec
+  commons-cli:commons-cli
+  commons-io:commons-io
+  commons-lang:commons-lang
+  commons-logging:commons-logging
+  com.fasterxml.jackson.core:jackson-core
+  
com.fasterxml.jackson.core:jackson-databind
+  
com.fasterxml.jackson.core:jackson-annotations
+  com.google.guava:guava
+  com.google.protobuf:protobuf-java
+  io.netty:netty
+  io.netty:netty-all
+  io.netty:netty-buffer
+  io.netty:netty-common
+  io.netty:netty-tcnative-boringssl-static
+  net.java.dev.jna:jna
+  net.jpountz.lz4:lz4
+  org.apache.bookkeeper:bookkeeper-common
+  org.apache.bookkeeper:bookkeeper-proto
+  org.apache.bookkeeper:bookkeeper-server
+  org.apache.bookkeeper:circe-checksum
+  org.apache.bookkeeper.http:http-server
+  
org.apache.bookkeeper.stats:bookkeeper-stats-api
+  org.apache.commons:commons-collections4
+  org.apache.commons:commons-lang3
+  
org.apache.distributedlog:distributedlog-common
+  
org.apache.distributedlog:distributedlog-core
+  
org.apache.distributedlog:distributedlog-protocol
+  org.apache.httpcomponents:httpclient
+  org.apache.httpcomponents:httpcore
+  org.apache.thrift:libthrift
+  org.apache.zookeeper:zookeeper
+  org.rocksdb:rocksdbjni
+
+  
+  
+
+
+  org.apache.commons.cli
+  dlshade.org.apache.commons.cli
+
+
+  org.apache.commons.codec
+  
dlshade.org.apache.commons.codec
+
+
+  org.a

[GitHub] sijie closed pull request #1096: BP-26 (task 4): run dlog tests when pull requests modify dlog modules

[GitBox]

sijie closed pull request #1096: BP-26 (task 4): run dlog tests when pull 
requests modify dlog modules
URL: https://github.com/apache/bookkeeper/pull/1096
 
 
   

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/.test-infra/jenkins/job_bookkeeper_postcommit_master_java8.groovy 
b/.test-infra/jenkins/job_bookkeeper_postcommit_master_java8.groovy
index 941bbfe4c..d4928bbc2 100644
--- a/.test-infra/jenkins/job_bookkeeper_postcommit_master_java8.groovy
+++ b/.test-infra/jenkins/job_bookkeeper_postcommit_master_java8.groovy
@@ -42,5 +42,5 @@ mavenJob('bookkeeper_postcommit_master_java8') {
   common_job_properties.setMavenConfig(delegate)
 
   // Maven build project.
-  goals('clean apache-rat:check package spotbugs:check')
+  goals('clean apache-rat:check package spotbugs:check -Ddistributedlog')
 }
diff --git a/.test-infra/jenkins/job_bookkeeper_postcommit_master_java9.groovy 
b/.test-infra/jenkins/job_bookkeeper_postcommit_master_java9.groovy
index 20e3e559a..613441367 100644
--- a/.test-infra/jenkins/job_bookkeeper_postcommit_master_java9.groovy
+++ b/.test-infra/jenkins/job_bookkeeper_postcommit_master_java9.groovy
@@ -42,5 +42,5 @@ mavenJob('bookkeeper_postcommit_master_java9') {
   common_job_properties.setMavenConfig(delegate)
 
   // Maven build project.
-  goals('clean apache-rat:check package spotbugs:check')
+  goals('clean apache-rat:check package spotbugs:check -Ddistributedlog')
 }
diff --git a/.test-infra/jenkins/job_bookkeeper_release_nightly_snapshot.groovy 
b/.test-infra/jenkins/job_bookkeeper_release_nightly_snapshot.groovy
index b6fb3bd5d..545e9c66b 100644
--- a/.test-infra/jenkins/job_bookkeeper_release_nightly_snapshot.groovy
+++ b/.test-infra/jenkins/job_bookkeeper_release_nightly_snapshot.groovy
@@ -41,5 +41,5 @@ mavenJob('bookkeeper_release_nightly_snapshot') {
   common_job_properties.setMavenConfig(delegate)
 
   // Maven build project.
-  goals('clean apache-rat:check package spotbugs:check 
-Dmaven.test.failure.ignore=true deploy')
+  goals('clean apache-rat:check package spotbugs:check 
-Dmaven.test.failure.ignore=true deploy -Ddistributedlog')
 }
diff --git a/.travis.yml b/.travis.yml
index 96bc636ad..f3dd4fa37 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -36,14 +36,29 @@ matrix:
   env: CUSTOM_JDK="openjdk8"
 
 before_install:
-  - echo "MAVEN_OPTS='-Xmx3072m -XX:MaxPermSize=512m'" > ~/.mavenrc
-  - if [ "$TRAVIS_OS_NAME" == "osx" ]; then export 
JAVA_HOME=$(/usr/libexec/java_home); fi
-  - if [ "$TRAVIS_OS_NAME" == "linux" ]; then jdk_switcher use "$CUSTOM_JDK"; 
fi
+- |
+echo "MAVEN_OPTS='-Xmx3072m -XX:MaxPermSize=512m'" > ~/.mavenrc
+if [ "$TRAVIS_OS_NAME" == "osx" ]; then
+export JAVA_HOME=$(/usr/libexec/java_home);
+fi
+if [ "$TRAVIS_OS_NAME" == "linux" ]; then
+jdk_switcher use "$CUSTOM_JDK";
+fi
+if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then
+export DLOG_MODIFIED="true"  
+echo "Enable testing distributedlog modules since they are not pull 
requests."
+else
+if [ `git diff --name-only $TRAVIS_COMMIT_RANGE | grep 
"^stream\/distributedlog" | wc -l` -gt 0 ]; then
+export DLOG_MODIFIED="true"  
+echo "Enable testing distributedlog modules if this pull request 
modifies files under directory `stream/distributedlog`."
+fi
+fi
 
 script:
-  - travis_retry mvn --batch-mode clean apache-rat:check compile 
spotbugs:check package -DskipTests
+  - travis_retry mvn --batch-mode clean apache-rat:check compile 
spotbugs:check install -DskipTests
   - if [ "$TRAVIS_OS_NAME" == "linux" ]; then dev/check-binary-license 
./bookkeeper-dist/all/target/bookkeeper-all-4.7.0-SNAPSHOT-bin.tar.gz; fi
   - if [ "$TRAVIS_OS_NAME" == "linux" ]; then dev/check-binary-license 
./bookkeeper-dist/server/target/bookkeeper-server-4.7.0-SNAPSHOT-bin.tar.gz; fi
+  - if [ "$DLOG_MODIFIED" == "true" ]; then cd stream/distributedlog && mvn 
--batch-mode clean package -Ddistributedlog; fi
 # Disabled the tests here. Since tests are running much slower on Travis than 
on Jenkins
 #  - ./dev/ticktoc.sh "mvn --batch-mode clean package"
 
diff --git a/stream/distributedlog/pom.xml b/stream/distributedlog/pom.xml
index 545ebaa0f..854a3394e 100644
--- a/stream/distributedlog/pom.xml
+++ b/stream/distributedlog/pom.xml
@@ -239,4 +239,5 @@
   
 
   
+
 


 


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 #1097: Use loopback network interface for testcases.

[GitBox]

sijie closed pull request #1097: Use loopback network interface for testcases.
URL: https://github.com/apache/bookkeeper/pull/1097
 
 
   

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-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
 
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
index dbfb9abf1..36406ce74 100644
--- 
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
+++ 
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
@@ -155,7 +155,7 @@ public void testBookieRegistrationWithSameZooKeeperClient() 
throws Exception {
 final ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
 conf.setJournalDirName(tmpDir.getPath())
 .setLedgerDirNames(new String[] { tmpDir.getPath() })
-.setZkServers(null);
+.setZkServers(null).setListeningInterface(null);
 
 final String bkRegPath = conf.getZkAvailableBookiesPath() + "/"
 + InetAddress.getLocalHost().getHostAddress() + ":"
@@ -188,7 +188,7 @@ public void testBookieRegistration() throws Exception {
 final ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
 conf.setJournalDirName(tmpDir.getPath())
 .setLedgerDirNames(new String[] { tmpDir.getPath() })
-.setZkServers(null);
+.setZkServers(null).setListeningInterface(null);
 
 final String bkRegPath = conf.getZkAvailableBookiesPath() + "/"
 + InetAddress.getLocalHost().getHostAddress() + ":"
@@ -250,7 +250,7 @@ public void 
testBookieRegistrationWithFQDNHostNameAsBookieID() throws Exception
 
 final ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration().setZkServers(null)
 .setJournalDirName(tmpDir.getPath()).setLedgerDirNames(new 
String[] { tmpDir.getPath() })
-.setUseHostNameAsBookieID(true);
+.setUseHostNameAsBookieID(true).setListeningInterface(null);
 
 final String bkRegPath = conf.getZkAvailableBookiesPath() + "/"
 + InetAddress.getLocalHost().getCanonicalHostName() + ":" + 
conf.getBookiePort();
@@ -271,7 +271,7 @@ public void 
testBookieRegistrationWithShortHostNameAsBookieID() throws Exception
 
 final ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration().setZkServers(null)
 .setJournalDirName(tmpDir.getPath()).setLedgerDirNames(new 
String[] { tmpDir.getPath() })
-.setUseHostNameAsBookieID(true).setUseShortHostName(true);
+
.setUseHostNameAsBookieID(true).setUseShortHostName(true).setListeningInterface(null);
 
 final String bkRegPath = conf.getZkAvailableBookiesPath() + "/"
 + 
(InetAddress.getLocalHost().getCanonicalHostName().split("\\.", 2)[0]) + ":" + 
conf.getBookiePort();
@@ -298,7 +298,8 @@ public void testRegNodeExistsAfterSessionTimeOut() throws 
Exception {
 final ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
 conf.setJournalDirName(tmpDir.getPath())
 .setLedgerDirNames(new String[] { tmpDir.getPath() })
-.setZkServers(zkUtil.getZooKeeperConnectString());
+.setZkServers(zkUtil.getZooKeeperConnectString())
+.setListeningInterface(null);
 
 String bkRegPath = conf.getZkAvailableBookiesPath() + "/"
 + InetAddress.getLocalHost().getHostAddress() + ":"
diff --git 
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/ConversionRollbackTest.java
 
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/ConversionRollbackTest.java
index 75e061a27..e3cec7c3c 100644
--- 
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/ConversionRollbackTest.java
+++ 
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/ConversionRollbackTest.java
@@ -40,6 +40,7 @@
 import org.apache.bookkeeper.bookie.InterleavedLedgerStorage;
 import org.apache.bookkeeper.bookie.LedgerDirsManager;
 import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.conf.TestBKConfiguration;
 import org.apache.bookkeeper.stats.NullStatsLogger;
 import org.apache.bookkeeper.util.DiskChecker;
 import org.apache.commons.io.FileUtils;
@@ -81,9 +82,8 @@ public void convertFromDbStorageToInterleaved() throws 
Exception {
 
 log.info("Using temp directory: {}", tmpDir);
 
-ServerConfiguration conf = new ServerConfiguration();
+ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
 conf.setL

[GitHub] sijie closed pull request #1104: Revamped downloads page

[GitBox]

sijie closed pull request #1104: Revamped downloads page
URL: https://github.com/apache/bookkeeper/pull/1104
 
 
   

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/Gemfile b/site/Gemfile
index 9ac4c81c6..e0953d64e 100644
--- a/site/Gemfile
+++ b/site/Gemfile
@@ -2,10 +2,6 @@ source 'https://rubygems.org'
 
 ruby '2.4.1'
 
-gem 'jekyll', '3.4.3'
-gem 'nokogiri', '1.8.1'
-gem 'jekyll-toc'
-
-group :jekyll_plugins do
-  gem 'jekyll-livereload', '0.2.2'
-end
+gem 'jekyll', '3.7.0'
+gem 'nokogiri'
+gem 'jekyll-toc', '0.2.1'
diff --git a/site/Gemfile.lock b/site/Gemfile.lock
index e219637e6..a9c775a04 100644
--- a/site/Gemfile.lock
+++ b/site/Gemfile.lock
@@ -1,66 +1,73 @@
 GEM
   remote: https://rubygems.org/
   specs:
-addressable (2.5.1)
-  public_suffix (~> 2.0, >= 2.0.2)
+addressable (2.5.2)
+  public_suffix (>= 2.0.2, < 4.0)
 colorator (1.1.0)
+concurrent-ruby (1.0.5)
 em-websocket (0.5.1)
   eventmachine (>= 0.12.9)
   http_parser.rb (~> 0.6.0)
-eventmachine (1.2.3)
+eventmachine (1.2.5)
 ffi (1.9.18)
 forwardable-extended (2.6.0)
 http_parser.rb (0.6.0)
-jekyll (3.4.3)
+i18n (0.9.3)
+  concurrent-ruby (~> 1.0)
+jekyll (3.7.0)
   addressable (~> 2.4)
   colorator (~> 1.0)
+  em-websocket (~> 0.5)
+  i18n (~> 0.7)
   jekyll-sass-converter (~> 1.0)
-  jekyll-watch (~> 1.1)
-  kramdown (~> 1.3)
-  liquid (~> 3.0)
+  jekyll-watch (~> 2.0)
+  kramdown (~> 1.14)
+  liquid (~> 4.0)
   mercenary (~> 0.3.3)
   pathutil (~> 0.9)
-  rouge (~> 1.7)
+  rouge (>= 1.7, < 4)
   safe_yaml (~> 1.0)
-jekyll-livereload (0.2.2)
-  em-websocket (~> 0.5)
-  jekyll (~> 3.0)
-jekyll-sass-converter (1.5.0)
+jekyll-sass-converter (1.5.1)
   sass (~> 3.4)
 jekyll-toc (0.2.1)
   nokogiri (~> 1.6)
-jekyll-watch (1.5.0)
-  listen (~> 3.0, < 3.1)
-kramdown (1.13.2)
-liquid (3.0.6)
-listen (3.0.8)
+jekyll-watch (2.0.0)
+  listen (~> 3.0)
+kramdown (1.16.2)
+liquid (4.0.0)
+listen (3.1.5)
   rb-fsevent (~> 0.9, >= 0.9.4)
   rb-inotify (~> 0.9, >= 0.9.7)
+  ruby_dep (~> 1.2)
 mercenary (0.3.6)
 mini_portile2 (2.3.0)
 nokogiri (1.8.1)
   mini_portile2 (~> 2.3.0)
-pathutil (0.14.0)
+pathutil (0.16.1)
   forwardable-extended (~> 2.6)
-public_suffix (2.0.5)
-rb-fsevent (0.9.8)
+public_suffix (3.0.1)
+rb-fsevent (0.10.2)
 rb-inotify (0.9.10)
   ffi (>= 0.5.0, < 2)
-rouge (1.11.1)
+rouge (3.1.1)
+ruby_dep (1.5.0)
 safe_yaml (1.0.4)
-sass (3.4.24)
+sass (3.5.5)
+  sass-listen (~> 4.0.0)
+sass-listen (4.0.0)
+  rb-fsevent (~> 0.9, >= 0.9.4)
+  rb-inotify (~> 0.9, >= 0.9.7)
 
 PLATFORMS
   ruby
 
 DEPENDENCIES
-  jekyll (= 3.4.3)
-  jekyll-livereload (= 0.2.2)
-  jekyll-toc
-  nokogiri (= 1.8.1)
+  jekyll (= 3.7.0)
+  jekyll-toc (= 0.2.1)
+  nokogiri
 
 RUBY VERSION
ruby 2.4.1p111
 
 BUNDLED WITH
-   1.15.1
+   1.16.1
diff --git a/site/Makefile b/site/Makefile
index a9090a1de..346de7dc1 100644
--- a/site/Makefile
+++ b/site/Makefile
@@ -1,6 +1,5 @@
-BUNDLER_VERSION = 1.15.1
-BUNDLE = bundle _${BUNDLER_VERSION}_
-JEKYLL = ${BUNDLE} exec jekyll
+BUNDLE := bundle
+JEKYLL := $(BUNDLE) exec jekyll
 
 dev:
code .
@@ -12,18 +11,17 @@ clean:
 
 setup:
gem install bundler \
-   -v ${BUNDLER_VERSION} \
--no-rdoc \
--no-ri
-   NOKOGIRI_USE_SYSTEM_LIBRARIES=true ${BUNDLE} install \
+   NOKOGIRI_USE_SYSTEM_LIBRARIES=true $(BUNDLE) install \
--path vendor/bundle
 
 build: clean
-   ${JEKYLL} build \
+   $(JEKYLL) build \
--config _config.yml
 
 apache: clean
-   JEKYLL_ENV=production ${JEKYLL} build \
+   JEKYLL_ENV=production $(JEKYLL) build \
--config _config.yml,_config.apache.yml
 
 javadoc:
@@ -33,10 +31,10 @@ latest_javadoc:
scripts/javadoc-gen.sh "latest"
 
 staging: clean
-   ${JEKYLL} build --config _config.yml,_config.staging.yml
+   $(JEKYLL) build --config _config.yml,_config.staging.yml
 
 serve: build
-   ${JEKYLL} serve \
+   $(JEKYLL) serve \
--incremental \
--livereload \
--config _config.yml,_config.local.yml
diff --git a/site/_config.yml b/site/_config.yml
index 85961edbf..9a059c36e 100644
--- a/site/_config.yml
+++ b/site/_config.yml
@@ -7,11 +7,9 @@ baseurl: /
 destination: local-generated
 twitter_url: https://twitter.com/asfbookkeeper
 
-livereload: true
-
 versions:
 - "4.6.1"
-# [next_version_placehodler]
+# [next_version_placeholder]
 - "4.6.0"
 - "4.5.1"
 - "4.5.0"
diff --git a/site/docs/4.

[GitHub] sijie closed pull request #1112: Update release schedule

[GitBox]

sijie closed pull request #1112: Update release schedule
URL: https://github.com/apache/bookkeeper/pull/1112
 
 
   

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/community/release_guide.md b/site/community/release_guide.md
index cded8281d..d7b01d2fe 100644
--- a/site/community/release_guide.md
+++ b/site/community/release_guide.md
@@ -518,6 +518,13 @@ In JIRA, inside [version 
management](https://issues.apache.org/jira/plugins/serv
 
 In Github, inside 
[milestones](https://github.com/apache/bookkeeper/milestones), hover over the 
current milestone and click `close` button to close a milestone and set today's 
date as due-date.
 
+### Update Release Schedule
+
+Update the [release schedule](../releases) page (only do this for feature 
release):
+
+- Bump the next feature release version and update its release window.
+- Update the release schedule to remove released version and add a new release.
+
 ### Checklist to proceed to the next step
 
 * Maven artifacts released and indexed in the [Maven Central 
Repository](https://search.maven.org/#search%7Cga%7C1%7Cg%3A%22org.apache.bookkeeper%22)
@@ -527,6 +534,7 @@ In Github, inside 
[milestones](https://github.com/apache/bookkeeper/milestones),
 * Release tagged in the source code repository
 * Release version finalized in JIRA and Github
 * Release section with release summary is added in 
[releases.md](https://github.com/apache/bookkeeper/blob/master/site/releases.md)
+* Release schedule page is updated
 
 **
 
diff --git a/site/community/releases.md b/site/community/releases.md
index ac839ec85..1a049bc9c 100644
--- a/site/community/releases.md
+++ b/site/community/releases.md
@@ -15,19 +15,19 @@ Apache BookKeeper community makes a feture release every 3 
month.
 
 ### Feature Release Window
 
-The next feature release is `4.6.0`. The release window is the following:
+The next feature release is `4.7.0`. The release window is the following:
 
 | **Date** | **Event** |
-| August 12, 2017 | Merge window opens on master branch |
-| October 16, 2017 | Major feature should be in, Cut release branch |
-| October 23, 2017 | Minor feature should be in, Stabilize release branch |
-| October 30, 2017 - November 12, 2017 | Code freeze, Only accept fixes for 
blocking issues, Rolling out release candidates |
+| November 12, 2017 | Merge window opens on master branch |
+| February 12, 2018 | Major feature should be in, Cut release branch |
+| February 19, 2018 | Minor feature should be in, Stabilize release branch |
+| February 26, 2018 - March 2, 2018 | Code freeze, Only accept fixes for 
blocking issues, Rolling out release candidates |
 
 ## Release Schedule
 
-- **4.6.0**: August 2017 - November 2017
 - **4.7.0**: November 2017 - February 2018
 - **4.8.0**: Februrary 2018 - May 2018
 - **4.9.0**: May 2018 - August 2018
+- **4.10.0**: August 2018 - November 2018
 
 https://calendar.google.com/calendar/embed?src=aam1p2gcoa40n68a6duflnva7c%40group.calendar.google.com&ctz=America/Los_Angeles";
 style="border: 0" width="800" height="600" frameborder="0" 
scrolling="no">


 


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 #1085: [Merge Yahoo repo]: YBK-160: Doing distributed random verification of ledger fragments

[GitBox]

sijie commented on issue #1085: [Merge Yahoo repo]: YBK-160: Doing distributed 
random verification of ledger fragments
URL: https://github.com/apache/bookkeeper/pull/1085#issuecomment-362752654
 
 
   @eolivelli can you review this again? @jiazhai already addressed your 
comment.
   
   @revans2 @govind-menon can you guys spend time on reviewing this one?


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 #1100: Improve decommission test

[GitBox]

sijie commented on issue #1100: Improve decommission test
URL: https://github.com/apache/bookkeeper/pull/1100#issuecomment-362753771
 
 
   @reddycharan since this branch depends on #1099 . I will address your 
comments here as we discussed offline after #1099 is in.


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 #1118: Travis CI should not use a hardcoded release version for check-release script

[GitBox]

sijie opened a new pull request #1118: Travis CI should not use a hardcoded 
release version for check-release script
URL: https://github.com/apache/bookkeeper/pull/1118
 
 
   
   Descriptions of the changes in this PR:
   
   CI is going to be broken when we bumped to 4.8.0.  since travis CI is using 
a hardcoded version for check-release script.


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 #1117: BP-29: Metadata API module

[GitBox]

jiazhai commented on a change in pull request #1117: BP-29: Metadata API module
URL: https://github.com/apache/bookkeeper/pull/1117#discussion_r165799985
 
 

 ##
 File path: site/bps/BP-29-metadata-store-api-module.md
 ##
 @@ -0,0 +1,87 @@
+---
+title: "BP-29: Metadata API module"
+issue: https://github.com/apache/bookkeeper/
+state: 'Under Discussion'
+release: "x.y.z"
+---
+
+### Motivation
+
+We have already abstracted all the metadata operations into interfaces. And 
all the bookkeeper implementations only reply on metadata interfaces,
+rather than depending on zookeeper. This proposal is to organize the metadata 
interfaces and its implementations in a separate module and make
+bookkeeper implementation only depends on metadata interfaces, not depends on 
zookeeper. This would a few benefits:
+
+- It allows supporting different metadata storages, without bringing in 
dependencies of metadata store implementation directly into
+  bookkeeper-server module. The development of different metadata storage can 
be done without interleaving with each other.
+- It would define a clean module dependency between bookkeeper implementation 
and metadata api, and how bookkeeper load a different metadata
+  implementation.
+
+### Public Interfaces
+
+A more generic setting `metadataServiceUri` is introduced for replacing 
implementation specific settings `zkServers` and `zkLedgersRootPath`.
+
+A metadata service uri defines the location of a metadata storage. In 
zookeeper based implementation, the metadata service url will be
+`zk:///`.
+
+This new setting in bookie configuration will be like as below:
+
+```
+metadataServiceUri=zk://127.0.0.1/ledgers
+```
+
+If we eventually support Etcd as one of the metadata storages. Then the 
setting in bookie configuration to use Etcd will be:
+
+```
+metadataServiceUri=etcd:///
+```
+
+### Proposed Changes
+
+ Configuration
+
+This BP proposes introducing a more generic metadata setting 
`metadataServiceUri` to replace implementation specific settings
+`zkServers` and `zkLedgersRootPath`. All implementation specific settings 
should be considered moving to implementation itself.
+
+The `metadataServiceUri` can also be used for replacing the need of 
configuring `ledgerManagerFactoryClass`, `registrationClientClass` and
+`registrationManagerClass`. It is unnecessarily complicated to configure 
multiple settings to load a specific metadata implementation.
+We can just use the `scheme` field in `metadataServiceUri` to resolve which 
metadata implementation to use. Using uri to resolve
 
 Review comment:
   nit: seems bring some line breaks because of  copy paste?


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 #1117: BP-29: Metadata API module

[GitBox]

jiazhai commented on a change in pull request #1117: BP-29: Metadata API module
URL: https://github.com/apache/bookkeeper/pull/1117#discussion_r165799985
 
 

 ##
 File path: site/bps/BP-29-metadata-store-api-module.md
 ##
 @@ -0,0 +1,87 @@
+---
+title: "BP-29: Metadata API module"
+issue: https://github.com/apache/bookkeeper/
+state: 'Under Discussion'
+release: "x.y.z"
+---
+
+### Motivation
+
+We have already abstracted all the metadata operations into interfaces. And 
all the bookkeeper implementations only reply on metadata interfaces,
+rather than depending on zookeeper. This proposal is to organize the metadata 
interfaces and its implementations in a separate module and make
+bookkeeper implementation only depends on metadata interfaces, not depends on 
zookeeper. This would a few benefits:
+
+- It allows supporting different metadata storages, without bringing in 
dependencies of metadata store implementation directly into
+  bookkeeper-server module. The development of different metadata storage can 
be done without interleaving with each other.
+- It would define a clean module dependency between bookkeeper implementation 
and metadata api, and how bookkeeper load a different metadata
+  implementation.
+
+### Public Interfaces
+
+A more generic setting `metadataServiceUri` is introduced for replacing 
implementation specific settings `zkServers` and `zkLedgersRootPath`.
+
+A metadata service uri defines the location of a metadata storage. In 
zookeeper based implementation, the metadata service url will be
+`zk:///`.
+
+This new setting in bookie configuration will be like as below:
+
+```
+metadataServiceUri=zk://127.0.0.1/ledgers
+```
+
+If we eventually support Etcd as one of the metadata storages. Then the 
setting in bookie configuration to use Etcd will be:
+
+```
+metadataServiceUri=etcd:///
+```
+
+### Proposed Changes
+
+ Configuration
+
+This BP proposes introducing a more generic metadata setting 
`metadataServiceUri` to replace implementation specific settings
+`zkServers` and `zkLedgersRootPath`. All implementation specific settings 
should be considered moving to implementation itself.
+
+The `metadataServiceUri` can also be used for replacing the need of 
configuring `ledgerManagerFactoryClass`, `registrationClientClass` and
+`registrationManagerClass`. It is unnecessarily complicated to configure 
multiple settings to load a specific metadata implementation.
+We can just use the `scheme` field in `metadataServiceUri` to resolve which 
metadata implementation to use. Using uri to resolve
 
 Review comment:
   nit: seems bring some line breaks because of  copy paste?
   also line 55,57,65,69


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 #1113: BP-28: Etcd as metadata store

[GitBox]

jiazhai commented on a change in pull request #1113: BP-28: Etcd as metadata 
store
URL: https://github.com/apache/bookkeeper/pull/1113#discussion_r165800469
 
 

 ##
 File path: site/bps/BP-28-etcd-as-metadata-store.md
 ##
 @@ -0,0 +1,102 @@
+---
+title: "BP-28: use etcd as metadata store"
+issue: https://github.com/apache/bookkeeper/
+state: 'Under Discussion'
+release: "N/A"
+---
+
+### Motivation
+
+Currently bookkeeper uses zookeeper as the metadata store. However there is a 
couple of issues with current approach, especially using zookeeper.
+
+These issues includes:
+
+1. You need to allocate special nodes for zookeeper. These nodes need to be 
treated specially, and have their own monitoring.
+   Ops need to understand both bookies and zookeeper.
+2. ZooKeeper is the scalability bottleneck. ZooKeeper doesn?t scale writes as 
you add nodes. This means that if your bookkeeper
+   cluster reaches the maximum write throughput that ZK can sustain, you?ve 
reached the maximum capacity of your cluster, and there?s nothing you
+   can do (except buy bigger hardware for your special nodes).
+3. ZooKeeper enforces you into its programming model. In general, its 
programming model is not too bad. However it becomes problematic when
+   the scale goes up (e.g. the number of clients and watcher increase). The 
issues usually comes from _session expires_ and _watcher_.
+  - *Session Expires*: For simplicity, ZooKeeper ties session state directly 
with connection state. So when a connection is broken, a session is usually 
expired (unless it reconnects before session expires), and when a session is 
expired, the underlying connection can not be used anymore, the application has 
to close the connection and re-establish a new client (a new connection). It is 
understandable that it makes zookeeper development easy. However in reality, it 
means if you can not establish a session, you can?t use this connection and you 
have to create new connections. Once your zookeeper cluster is in a bad state 
(e.g. network issue or jvm gc), the whole cluster is usually unable to recover 
because of the connection storm introduced by session expires.
+  - *Watchers*: The zookeeper watcher is one time watcher, applications can?t 
reliably use it to get updates. In order to set a watcher, you have to read a 
znode or get children. Imagine such a use case, clients are watching a list of 
znodes (e.g. list of bookies), when those clients expire, they have to get the 
list of znodes in order to rewatch the list, even the list is never changed.
+  - The combination of session expires and watchers is often the root cause of 
critical zookeeper outages.
+
+This proposal is to explore other existing systems such as etcd as the 
metadata store. Using Etcd doesn't address concerns #1, however it might 
potentially
+address concern #2 and #3 to some extend. And if you are running bookkeeper in 
k8s, there is already an Etcd instance available. It can become easier to run
+bookkeeper on k8s if we can use Etcd as the metadata store.
 
 Review comment:
   nit: seems bring some line breaks because of copy paste?


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 #1111: Update PR template

[GitBox]

sijie closed pull request #: Update PR template
URL: https://github.com/apache/bookkeeper/pull/
 
 
   

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/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
index 6a0d6a6dd..b9558fadc 100644
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -19,10 +19,9 @@ Master Issue: #
 > Otherwise:
 > 
 > - [ ] Make sure the PR title is formatted like:
-> `: Description of pull request`
+> `: Description of pull request`
 > `e.g. Issue 123: Description ...`
-> `e.g. BOOKKEEPER-1234: Description ...`
 > - [ ] Make sure tests pass via `mvn clean apache-rat:check install 
 > spotbugs:check`.
-> - [ ] Replace `` in the title with the actual 
Issue/JIRA number.
+> - [ ] Replace `` in the title with the actual Issue 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] sijie commented on issue #1118: Travis CI should not use a hardcoded release version for check-release script

[GitBox]

sijie commented on issue #1118: Travis CI should not use a hardcoded release 
version for check-release script
URL: https://github.com/apache/bookkeeper/pull/1118#issuecomment-362782237
 
 
   retest this please


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 #1094: BP-27: New BookKeeper CLI

[GitBox]

sijie commented on issue #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-362783195
 
 
   This change is ready to review 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] sijie commented on issue #1094: BP-27: New BookKeeper CLI

[GitBox]

sijie commented on issue #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-362783315
 
 
   @jvrao @reddycharan: since you guys contributed a lot of shell commands, it 
would be great that you guys can spend some time on reviewing this one. 


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