[GitHub] ivankelly commented on issue #1131: Split all versions images into released and latest

[GitBox]

ivankelly commented on issue #1131: Split all versions images into released and 
latest
URL: https://github.com/apache/bookkeeper/pull/1131#issuecomment-365544555
 
 
   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] ivankelly commented on issue #1131: Split all versions images into released and latest

[GitBox]

ivankelly commented on issue #1131: Split all versions images into released and 
latest
URL: https://github.com/apache/bookkeeper/pull/1131#issuecomment-365596160
 
 
   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

Jenkins build is still unstable: bookkeeper_release_branch #59

[Apache Jenkins Server]

See 

[GitHub] ivankelly commented on issue #1142: Gather disk information before CI build

[GitBox]

ivankelly commented on issue #1142: Gather disk information before CI build
URL: https://github.com/apache/bookkeeper/pull/1142#issuecomment-365614153
 
 
   @sijie @eolivelli I checked with the infra folks on hipchat. they say it's 
fine


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 still unstable: bookkeeper_postcommit_master_java8 #41

[Apache Jenkins Server]

See 

Jenkins build is back to stable : bookkeeper_release_nightly_snapshot #59

[Apache Jenkins Server]

See 

Jenkins build is back to stable : bookkeeper_postcommit_master_java9 #42

[Apache Jenkins Server]

See 

[GitHub] ivankelly closed pull request #1131: Split all versions images into released and latest

[GitBox]

ivankelly closed pull request #1131: Split all versions images into released 
and latest
URL: https://github.com/apache/bookkeeper/pull/1131
 
 
   

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/tests/docker-all-versions-image/Dockerfile 
b/tests/docker-images/all-released-versions-image/Dockerfile
similarity index 85%
rename from tests/docker-all-versions-image/Dockerfile
rename to tests/docker-images/all-released-versions-image/Dockerfile
index c389d2d6b..b0ac480f2 100644
--- a/tests/docker-all-versions-image/Dockerfile
+++ b/tests/docker-images/all-released-versions-image/Dockerfile
@@ -17,16 +17,16 @@
 # under the License.
 #
 
-FROM centos:7
+FROM openjdk:8-jdk
 MAINTAINER Apache BookKeeper 
 
-ARG BK_TARBALL=DOESNOTEXIST
 ENV BK_JOURNALDIR=/opt/bookkeeper/data/journal
 ENV BK_LEDGERDIR=/opt/bookkeeper/data/ledgers
 ENV BK_ZKCONNECTSTRING=zookeeper1,zookeeper2,zookeeper3
 
-RUN yum install -y 
https://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm
-RUN yum install -y java-1.8.0-openjdk-headless wget bash python sudo 
supervisor which
+RUN apt-get update && apt-get install -qy wget supervisor bash \
+&& echo "dash dash/sh boolean false" | debconf-set-selections \
+&& DEBIAN_FRONTEND=noninteractive dpkg-reconfigure dash
 
 RUN mkdir /tarballs
 WORKDIR /tarballs
@@ -45,14 +45,16 @@ RUN wget -nv 
https://archive.apache.org/dist/bookkeeper/bookkeeper-4.4.0/bookkee
 RUN wget -nv 
https://archive.apache.org/dist/bookkeeper/bookkeeper-4.5.0/bookkeeper-server-4.5.0-bin.tar.gz{,.sha1,.md5,.asc}
 RUN wget -nv 
https://archive.apache.org/dist/bookkeeper/bookkeeper-4.5.1/bookkeeper-server-4.5.1-bin.tar.gz{,.sha1,.md5,.asc}
 RUN wget -nv 
https://archive.apache.org/dist/bookkeeper/bookkeeper-4.6.0/bookkeeper-server-4.6.0-bin.tar.gz{,.sha1,.md5,.asc}
+RUN wget -nv 
https://archive.apache.org/dist/bookkeeper/bookkeeper-4.6.1/bookkeeper-server-4.6.1-bin.tar.gz{,.sha1,.md5,.asc}
 RUN wget -nv https://dist.apache.org/repos/dist/release/bookkeeper/KEYS
 RUN wget -nv 
http://svn.apache.org/repos/asf/zookeeper/bookkeeper/dist/KEYS?p=1620552 -O 
KEYS.old
-COPY ${BK_TARBALL} /tarballs/
 
+RUN mkdir -p /etc/supervisord/conf.d && mkdir -p /var/run/supervisor && mkdir 
-p /var/log/bookkeeper
+ADD conf/supervisord.conf /etc/supervisord.conf
 ADD scripts/install-all-tarballs.sh /install-all-tarballs.sh
+ADD scripts/install-tarball.sh /install-tarball.sh
 RUN bash /install-all-tarballs.sh && rm -rf /tarballs
 
 WORKDIR /
 ADD scripts/update-conf-and-boot.sh /update-conf-and-boot.sh
-RUN chmod +x /update-conf-and-boot.sh
 CMD ["/update-conf-and-boot.sh"]
\ No newline at end of file
diff --git 
a/tests/docker-images/all-released-versions-image/conf/supervisord.conf 
b/tests/docker-images/all-released-versions-image/conf/supervisord.conf
new file mode 100644
index 0..da124af7c
--- /dev/null
+++ b/tests/docker-images/all-released-versions-image/conf/supervisord.conf
@@ -0,0 +1,38 @@
+#/**
+# * 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.
+# */
+[supervisord]
+nodaemon=true
+logfile=/var/log/supervisord.log
+logfile_maxbytes=50MB
+logfile_backups=10
+loglevel=info
+pidfile=/var/run/supervisord.pid
+minfds=1024
+minprocs=200
+
+[unix_http_server]
+file=/var/run/supervisor/supervisor.sock
+
+[supervisorctl]
+serverurl=unix:///var/run/supervisor/supervisor.sock
+
+[rpcinterface:supervisor]
+supervisor.rpcinterface_factory = 
supervisor.rpcinterface:make_main_rpcinterface
+
+[include]
+files = /etc/supervisord/conf.d/*.conf
diff --git a/tests/docker-images/all-released-versions-image/pom.xml 
b/tests/docker-images/all-released-versions-image/pom.xml
new file mode 100644
index 0..3e629cfad
--- /dev/null
+++ b/tests/docker-images/all-released-versions-image/pom.xml
@@ -0,0 +1,72 @@
+
+
+http://maven.apache.org/POM/4.0.0"; 
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+  
+org.apache.

[GitHub] ivankelly commented on issue #1135: Specify repo in MavenClassLoader

[GitBox]

ivankelly commented on issue #1135: Specify repo in MavenClassLoader
URL: https://github.com/apache/bookkeeper/pull/1135#issuecomment-365638484
 
 
   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] ivankelly commented on issue #1142: Gather disk information before CI build

[GitBox]

ivankelly commented on issue #1142: Gather disk information before CI build
URL: https://github.com/apache/bookkeeper/pull/1142#issuecomment-365664712
 
 
   Merging this. 


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


With regards,
Apache Git Services

[GitHub] ivankelly closed pull request #1142: Gather disk information before CI build

[GitBox]

ivankelly closed pull request #1142: Gather disk information before CI build
URL: https://github.com/apache/bookkeeper/pull/1142
 
 
   

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_precommit_java8.groovy 
b/.test-infra/jenkins/job_bookkeeper_precommit_java8.groovy
index 5b6be305b..82aa4b794 100644
--- a/.test-infra/jenkins/job_bookkeeper_precommit_java8.groovy
+++ b/.test-infra/jenkins/job_bookkeeper_precommit_java8.groovy
@@ -22,6 +22,15 @@ import common_job_properties
 mavenJob('bookkeeper_precommit_pullrequest_java8') {
   description('precommit verification for pull requests of http://bookkeeper.apache.org";>Apache BookKeeper in Java 8.')
 
+  // Temporary information gathering to see if full disks are causing the 
builds to flake
+  preBuildSteps {
+shell("id")
+shell("ulimit -a")
+shell("pwd")
+shell("df -h")
+shell("ps aux")
+  }
+
   // Execute concurrent builds if necessary.
   concurrentBuild()
 
diff --git a/.test-infra/jenkins/job_bookkeeper_precommit_java9.groovy 
b/.test-infra/jenkins/job_bookkeeper_precommit_java9.groovy
index d0cea2b2c..ad803beed 100644
--- a/.test-infra/jenkins/job_bookkeeper_precommit_java9.groovy
+++ b/.test-infra/jenkins/job_bookkeeper_precommit_java9.groovy
@@ -22,6 +22,15 @@ import common_job_properties
 mavenJob('bookkeeper_precommit_pullrequest_java9') {
   description('precommit verification for pull requests of http://bookkeeper.apache.org";>Apache BookKeeper in Java 9.')
 
+  // Temporary information gathering to see if full disks are causing the 
builds to flake
+  preBuildSteps {
+shell("id")
+shell("ulimit -a")
+shell("pwd")
+shell("df -h")
+shell("ps aux")
+  }
+
   // Execute concurrent builds if necessary.
   concurrentBuild()
 


 


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] kishorvpatil opened a new pull request #1125: Issue #1124: Lower memory usage in GarbageCollectionThread while extracting all ledger meta data

[GitBox]

kishorvpatil opened a new pull request #1125: Issue #1124: Lower memory usage 
in GarbageCollectionThread while extracting all ledger meta data
URL: https://github.com/apache/bookkeeper/pull/1125
 
 
   Descriptions of the changes in this PR:
   
   The PR contains the fix to cleanup non-existent ledger log entries from 
EntryLogMetadata while extracting all log entries.
   
   Master Issue: #1124
   
   > ---
   > Be sure to do all of the following to help us incorporate your contribution
   > quickly and easily:
   >
   > If this PR is a BookKeeper Proposal (BP):
   >
   > - [ ] Make sure the PR title is formatted like:
   > `: Description of bookkeeper proposal`
   > `e.g. BP-1: 64 bits ledger is support`
   > - [ ] Attach the master issue link in the description of this PR.
   > - [ ] Attach the google doc link if the BP is written in Google Doc.
   >
   > Otherwise:
   > 
   > - [X] Make sure the PR title is formatted like:
   > `: Description of pull request`
   > `e.g. Issue 123: Description ...`
   > - [X] Make sure tests pass via `mvn clean apache-rat:check install 
spotbugs:check`.
   > - [X] 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 closed pull request #1125: Issue #1124: Lower memory usage in GarbageCollectionThread while extracting all ledger meta data

[GitBox]

sijie closed pull request #1125: Issue #1124: Lower memory usage in 
GarbageCollectionThread while extracting all ledger meta data
URL: https://github.com/apache/bookkeeper/pull/1125
 
 
   

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/bookie/GarbageCollectorThread.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
index 4880fa354..79515ea4b 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
@@ -389,16 +389,7 @@ private void doGcEntryLogs() {
 
 // Loop through all of the entry logs and remove the non-active 
ledgers.
 entryLogMetaMap.forEach((entryLogId, meta) -> {
-meta.removeLedgerIf((entryLogLedger) -> {
-// Remove the entry log ledger from the set if it isn't active.
-   try {
-   return !ledgerStorage.ledgerExists(entryLogLedger);
-   } catch (IOException e) {
-   LOG.error("Error reading from ledger storage", e);
-   return false;
-   }
-   });
-
+   removeIfLedgerNotExists(meta);
if (meta.isEmpty()) {
// This means the entry log is not associated with any active 
ledgers anymore.
// We can remove this entry log file now.
@@ -414,6 +405,18 @@ private void doGcEntryLogs() {
 this.numActiveEntryLogs = entryLogMetaMap.keySet().size();
 }
 
+private void removeIfLedgerNotExists(EntryLogMetadata meta) {
+meta.removeLedgerIf((entryLogLedger) -> {
+// Remove the entry log ledger from the set if it isn't active.
+try {
+return !ledgerStorage.ledgerExists(entryLogLedger);
+} catch (IOException e) {
+LOG.error("Error reading from ledger storage", e);
+return false;
+}
+});
+}
+
 /**
  * Compact entry logs if necessary.
  *
@@ -546,7 +549,12 @@ protected void compactEntryLog(EntryLogMetadata 
entryLogMeta) {
 try {
 // Read through the entry log file and extract the entry log 
meta
 EntryLogMetadata entryLogMeta = 
entryLogger.getEntryLogMetadata(entryLogId);
-entryLogMetaMap.put(entryLogId, entryLogMeta);
+removeIfLedgerNotExists(entryLogMeta);
+if (entryLogMeta.isEmpty()) {
+entryLogger.removeEntryLog(entryLogId);
+} else {
+entryLogMetaMap.put(entryLogId, entryLogMeta);
+}
 } catch (IOException e) {
 hasExceptionWhenScan = true;
 LOG.warn("Premature exception when processing " + entryLogId
diff --git 
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java
 
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java
index 6873a5c53..00c97c435 100644
--- 
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java
+++ 
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java
@@ -39,8 +39,10 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Enumeration;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -835,6 +837,74 @@ public void checkpointComplete(Checkpoint checkpoint,
 storage.gcThread.doCompactEntryLogs(threshold);
 }
 
+/**
+ * Test extractMetaFromEntryLogs optimized method to avoid excess memory 
usage.
+ */
+public void testExtractMetaFromEntryLogs() throws Exception {
+// Always run this test with Throttle enabled. 
+baseConf.setIsThrottleByBytes(true);
+// restart bookies
+restartBookies(baseConf);
+ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
+File tmpDir = createTempDir("bkTest", ".dir");
+File curDir = Bookie.getCurrentDirectory(tmpDir);
+Bookie.checkDirectoryStructure(curDir);
+conf.setLedgerDirNames(new String[] { tmpDir.toString() });
+
+LedgerDirsManager dirs = new LedgerDirsManager(conf, 
conf.getLedgerDirs(),
+new DiskChecker(conf.getDiskUsageThreshold(), 
conf.getDiskUsageWarnThreshold()));
+final Set ledgers = Collections
+.newSetFromMap(new ConcurrentHashMap());
+
+LedgerManager manager = getLedgerManager

[GitHub] sijie commented on issue #1125: Issue #1124: Lower memory usage in GarbageCollectionThread while extracting all ledger meta data

[GitBox]

sijie commented on issue #1125: Issue #1124: Lower memory usage in 
GarbageCollectionThread while extracting all ledger meta data
URL: https://github.com/apache/bookkeeper/pull/1125#issuecomment-365671775
 
 
   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 a change in pull request #1121: [Merge Yahoo repo]: AsyncReadLastEntry should trigger callback with error when ledger is empty

[GitBox]

sijie commented on a change in pull request #1121: [Merge Yahoo repo]: 
AsyncReadLastEntry should trigger callback with error when ledger is empty
URL: https://github.com/apache/bookkeeper/pull/1121#discussion_r168240944
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##
 @@ -787,6 +788,35 @@ public void onFailure(Throwable cause) {
 }
 }
 
+/*
+ * Read the last entry in the ledger
+ *
+ * @param cb
+ *object implementing read callback interface
+ * @param ctx
+ *control object
+ */
+public void asyncReadLastEntry(ReadCallback cb, Object ctx) {
+long lastEntryId = getLastAddConfirmed();
+if (lastEntryId < 0) {
+// Ledger was empty, so there is no last entry to read
+cb.readComplete(BKException.Code.NoSuchEntryException, this, null, 
ctx);
+} else {
+asyncReadEntriesInternal(lastEntryId, lastEntryId, cb, ctx);
+}
+}
+
+public org.apache.bookkeeper.client.api.LedgerEntry readLastEntry()
+throws InterruptedException, ExecutionException {
+long lastEntryId = getLastAddConfirmed();
+if (lastEntryId < 0) {
+// Ledger was empty, so there is no last entry to read
+return null;
 
 Review comment:
   throw NoSuchEntryException to keep it consistent with asyncReadLastEntry.


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 #1121: [Merge Yahoo repo]: AsyncReadLastEntry should trigger callback with error when ledger is empty

[GitBox]

sijie commented on a change in pull request #1121: [Merge Yahoo repo]: 
AsyncReadLastEntry should trigger callback with error when ledger is empty
URL: https://github.com/apache/bookkeeper/pull/1121#discussion_r168241554
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java
 ##
 @@ -178,4 +180,19 @@ public String toString() {
 protected void initializeExplicitLacFlushPolicy() {
 explicitLacFlushPolicy = 
ExplicitLacFlushPolicy.VOID_EXPLICITLAC_FLUSH_POLICY;
 }
+
+@Override
+public void asyncReadLastEntry(ReadCallback cb, Object ctx) {
+asyncReadLastConfirmed(new ReadLastConfirmedCallback() {
+@Override
+public void readLastConfirmedComplete(int rc, long lastConfirmed, 
Object ctx) {
+if (lastConfirmed < 0) {
 
 Review comment:
   you need check `rc` first. if `rc` is not `OK`, then fail `ReadCallback`. 
Otherwise do the logic to read actual entry.


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 #1140: Issue 1139: Add debug to replication fencing

[GitBox]

sijie commented on a change in pull request #1140: Issue 1139: Add debug to 
replication fencing
URL: https://github.com/apache/bookkeeper/pull/1140#discussion_r168245303
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java
 ##
 @@ -364,40 +368,52 @@ private void deferLedgerLockRelease(final long ledgerId) 
{
 TimerTask timerTask = new TimerTask() {
 @Override
 public void run() {
+boolean isRecoveryOpen = false;
 LedgerHandle lh = null;
 try {
 lh = admin.openLedgerNoRecovery(ledgerId);
 if (isLastSegmentOpenAndMissingBookies(lh)) {
+// Need recovery open, close the old ledger handle.
+lh.close();
+// Recovery open could result in client write failure.
+LOG.warn("Missing bookie(s) from last segment. Opening 
Ledger{} for Recovery.", ledgerId);
 lh = admin.openLedger(ledgerId);
+isRecoveryOpen = true;
 }
-
-Set fragments = 
getUnderreplicatedFragments(lh);
-for (LedgerFragment fragment : fragments) {
-if (!fragment.isClosed()) {
-lh = admin.openLedger(ledgerId);
-break;
+if (!isRecoveryOpen){
 
 Review comment:
   @jvrao I see.
   
   to make things a bit clear here, it might be good to move line 384 to 395 to 
a function call "hasOpenUnderreplicatedFragements(LedgerHandle lh)`.
   
   and change the if else here to be:
   
   ```
   if (isLastSegmentOpenAndMissingBookies(lh) || 
hasOpenUnderreplicatedFragments(lh)) {
 lh.close();
 lh = admin.openLedger(ledgerId);
 isRecoveryOpen = true;
   }
   ```
   
   not a blocker though.


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 #1130: handle zookeeper session expire in zk ledger manager

[GitBox]

sijie commented on issue #1130: handle zookeeper session expire in zk ledger 
manager
URL: https://github.com/apache/bookkeeper/pull/1130#issuecomment-365678080
 
 
   ping @jvrao 


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 #1127: BP-29 (task 1) : Introduce `metadataServiceUrl`

[GitBox]

sijie commented on issue #1127: BP-29 (task 1) : Introduce `metadataServiceUrl`
URL: https://github.com/apache/bookkeeper/pull/1127#issuecomment-365678201
 
 
   ping @jvrao 


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

[GitBox]

sijie commented on issue #1110: Issue 1109: Error out pending ops on TLS key 
mismatch exception
URL: https://github.com/apache/bookkeeper/pull/1110#issuecomment-365679452
 
 
   @jiazhai since you were in reviewing this PR, can you review this again 
since @kishorekasi 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] sijie commented on issue #1110: Issue 1109: Error out pending ops on TLS key mismatch exception

[GitBox]

sijie commented on issue #1110: Issue 1109: Error out pending ops on TLS key 
mismatch exception
URL: https://github.com/apache/bookkeeper/pull/1110#issuecomment-365680205
 
 
   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 #1108: Replace DoubleByteBuf with CompositeByteBuf because of perf regression with Netty > 4.1.12

[GitBox]

sijie 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-365680586
 
 
   @merlimat do you need any help to fix the checkstyle problem, so we can move 
forward with merging this PR.


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_r168249000
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java
 ##
 @@ -0,0 +1,176 @@
+/*
+ * 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.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import java.util.Iterator;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.bookie.Bookie;
+import org.apache.bookkeeper.client.BKException.BKIllegalOpException;
+import org.apache.bookkeeper.client.BookKeeper.DigestType;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.ZkLedgerUnderreplicationManager;
+import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
+import org.apache.bookkeeper.test.annotations.FlakyTest;
+import org.junit.Test;
+
+/**
+ * Unit test of bookie decommission operations.
+ */
+@Slf4j
+public class BookieDecommissionTest extends BookKeeperClusterTestCase {
+
+private static final int NUM_OF_BOOKIES = 6;
+private static DigestType digestType = DigestType.CRC32;
+private static final String PASSWORD = "testPasswd";
+
+public BookieDecommissionTest() {
+super(NUM_OF_BOOKIES, 480);
+baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(3));
+setAutoRecoveryEnabled(true);
+}
+
+@FlakyTest("https://github.com/apache/bookkeeper/issues/502";)
 
 Review comment:
   I have a subsequent change to remove FlakyTest at #1100


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

[GitBox]

sijie commented on issue #1099: Fix auditor shutdown logic and move decommision 
tests out of BookKeeperAdminTest
URL: https://github.com/apache/bookkeeper/pull/1099#issuecomment-365681168
 
 
   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 #1087: ISSUE #1075: Add a noop digest implentation

[GitBox]

sijie commented on issue #1087: ISSUE #1075: Add a noop digest implentation
URL: https://github.com/apache/bookkeeper/pull/1087#issuecomment-365681417
 
 
   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 #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-365681541
 
 
   ping @eolivelli 


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-365681563
 
 
   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] merlimat commented on issue #1108: Replace DoubleByteBuf with CompositeByteBuf because of perf regression with Netty > 4.1.12

[GitBox]

merlimat 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-365684412
 
 
   I'd close this one in favor of #1141


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


With regards,
Apache Git Services

[GitHub] merlimat commented on issue #1108: Replace DoubleByteBuf with CompositeByteBuf because of perf regression with Netty > 4.1.12

[GitBox]

merlimat 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-365684674
 
 
   (maybe this PR could be used for 4.6.2, but for 4.7.0 I'd rather go with the 
#1441 approach)


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

[GitBox]

sijie 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-365688896
 
 
   @merlimat yeah. this PR is good for 4.6.2. so if you can fix the checkstyle 
here, I can merge it to 4.7.0 and 4.6.2. and #1441 can come later in master for 
4.7.0. How does that sound?


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168261214
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/cluster/ListBookiesCommand.java
 ##
 @@ -0,0 +1,75 @@
+/*
+ * 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.tools.cli.commands.cluster;
+
+import static org.apache.bookkeeper.common.concurrent.FutureUtils.result;
+import static 
org.apache.bookkeeper.tools.cli.helpers.CommandHelpers.getBookieSocketAddrStringRepresentation;
+
+import com.beust.jcommander.Parameter;
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.discover.RegistrationClient;
+import org.apache.bookkeeper.net.BookieSocketAddress;
+import org.apache.bookkeeper.tools.cli.helpers.DiscoveryCommand;
+
+/**
+ * Command to list available bookies.
+ */
+@Accessors(fluent = true)
+@Setter
+@Parameters(commandDescription = "List the bookies, which are running as 
either readwrite or readonly mode.")
+public class ListBookiesCommand extends DiscoveryCommand {
+
+@Parameter(names = { "-rw", "--readwrite" }, description = "Print 
readwrite bookies")
+private boolean readwrite = false;
+@Parameter(names = { "-ro", "--readonly" }, description = "Print readonly 
bookies")
+private boolean readonly = false;
+
+@Override
+protected void run(RegistrationClient regClient) throws Exception {
+List bookies = Lists.newArrayList();
+if (readwrite) {
+bookies.addAll(
+result(
+regClient.getWritableBookies()
+).getValue()
+);
+} else if (readonly) {
+bookies.addAll(
+result(
+regClient.getReadOnlyBookies()
+).getValue()
+);
+}
 
 Review comment:
   done.
   
   1. add a `validateArgs` method to let each command validate args before 
actual running. 
   2. enforce "One and only one of --readwrite and --readonly must be specified"


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168261389
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/helpers/DiscoveryCommand.java
 ##
 @@ -0,0 +1,59 @@
+/*
+ * 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.tools.cli.helpers;
+
+import java.util.Optional;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import lombok.Cleanup;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.client.api.BookKeeper;
+import org.apache.bookkeeper.conf.ClientConfiguration;
+import org.apache.bookkeeper.discover.RegistrationClient;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.util.ReflectionUtils;
+
+/**
+ * This is a mixin for commands that talks to discovery service.
+ */
+@Slf4j
+public abstract class DiscoveryCommand extends ClientCommand {
+
+@Override
+protected void run(ClientConfiguration conf) throws Exception {
+Class regClientCls = 
conf.getRegistrationClientClass();
+@Cleanup("shutdown") ScheduledExecutorService executor = 
Executors.newSingleThreadScheduledExecutor();
+try (RegistrationClient regClient = 
ReflectionUtils.newInstance(regClientCls)) {
+regClient.initialize(
+conf,
+executor,
+NullStatsLogger.INSTANCE,
+Optional.empty());
+run(regClient);
+}
+}
+
+@Override
+protected void run(BookKeeper bk) throws Exception {
+throw new UnsupportedOperationException("Discovery command only uses 
registration client");
 
 Review comment:
   done. let `DiscoveryCommand` inherits `Command` directly, instead of 
`ClientCommand`.


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-365694176
 
 
   @reddycharan I have addressed your comments. please review it again.


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 #1135: Specify repo in MavenClassLoader

[GitBox]

sijie closed pull request #1135: Specify repo in MavenClassLoader
URL: https://github.com/apache/bookkeeper/pull/1135
 
 
   

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/tests/integration-tests-utils/src/main/java/org/apache/bookkeeper/tests/MavenClassLoader.java
 
b/tests/integration-tests-utils/src/main/java/org/apache/bookkeeper/tests/MavenClassLoader.java
index 6f31f5c76..a7091bec1 100644
--- 
a/tests/integration-tests-utils/src/main/java/org/apache/bookkeeper/tests/MavenClassLoader.java
+++ 
b/tests/integration-tests-utils/src/main/java/org/apache/bookkeeper/tests/MavenClassLoader.java
@@ -31,6 +31,7 @@
 import java.util.List;
 import java.util.Optional;
 
+import org.jboss.shrinkwrap.resolver.api.maven.ConfigurableMavenResolverSystem;
 import org.jboss.shrinkwrap.resolver.api.maven.Maven;
 import org.jboss.shrinkwrap.resolver.api.maven.ScopeType;
 import org.jboss.shrinkwrap.resolver.api.maven.coordinate.MavenDependencies;
@@ -48,8 +49,18 @@ private MavenClassLoader(URLClassLoader cl) {
 
 private final URLClassLoader classloader;
 
+public static MavenClassLoader forArtifact(String repo, String 
mainArtifact) throws Exception {
+return 
createClassLoader(Maven.configureResolver().withRemoteRepo("custom", repo, 
"default"),
+ mainArtifact);
+}
+
 public static MavenClassLoader forArtifact(String mainArtifact) throws 
Exception {
-Optional slf4jVersion = 
Arrays.stream(Maven.resolver().resolve(mainArtifact)
+return createClassLoader(Maven.configureResolver(), mainArtifact);
+}
+
+private static MavenClassLoader 
createClassLoader(ConfigurableMavenResolverSystem resolver,
+  String mainArtifact) 
throws Exception {
+Optional slf4jVersion = 
Arrays.stream(resolver.resolve(mainArtifact)
   
.withTransitivity().asResolvedArtifact())
 .filter((a) -> a.getCoordinate().getGroupId().equals("org.slf4j")
 && 
a.getCoordinate().getArtifactId().equals("slf4j-log4j12"))
@@ -66,7 +77,7 @@ public static MavenClassLoader forArtifact(String 
mainArtifact) throws Exception
 ScopeType.COMPILE, 
false));
 }
 
-File[] files = Maven.resolver().addDependencies(deps.toArray(new 
MavenDependency[0]))
+File[] files = resolver.addDependencies(deps.toArray(new 
MavenDependency[0]))
 .resolve().withTransitivity().asFile();
 URLClassLoader cl = AccessController.doPrivileged(
 new PrivilegedAction() {


 


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 #1133: Integration smoke test

[GitBox]

sijie commented on issue #1133: Integration smoke test
URL: https://github.com/apache/bookkeeper/pull/1133#issuecomment-365695215
 
 
   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] reddycharan commented on issue #1100: Improve decommission test

[GitBox]

reddycharan commented on issue #1100: Improve decommission test
URL: https://github.com/apache/bookkeeper/pull/1100#issuecomment-365696547
 
 
   LGTM +1


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


With regards,
Apache Git Services

[GitHub] yzang commented on a change in pull request #1094: BP-27: New BookKeeper CLI

[GitBox]

yzang commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168268922
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/cluster/ListBookiesCommand.java
 ##
 @@ -0,0 +1,75 @@
+/*
+ * 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.tools.cli.commands.cluster;
+
+import static org.apache.bookkeeper.common.concurrent.FutureUtils.result;
+import static 
org.apache.bookkeeper.tools.cli.helpers.CommandHelpers.getBookieSocketAddrStringRepresentation;
+
+import com.beust.jcommander.Parameter;
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.discover.RegistrationClient;
+import org.apache.bookkeeper.net.BookieSocketAddress;
+import org.apache.bookkeeper.tools.cli.helpers.DiscoveryCommand;
+
+/**
+ * Command to list available bookies.
+ */
+@Accessors(fluent = true)
+@Setter
+@Parameters(commandDescription = "List the bookies, which are running as 
either readwrite or readonly mode.")
+public class ListBookiesCommand extends DiscoveryCommand {
+
+@Parameter(names = { "-rw", "--readwrite" }, description = "Print 
readwrite bookies")
+private boolean readwrite = false;
+@Parameter(names = { "-ro", "--readonly" }, description = "Print readonly 
bookies")
+private boolean readonly = false;
+
+@Override
+protected void run(RegistrationClient regClient) throws Exception {
+List bookies = Lists.newArrayList();
+if (readwrite) {
+bookies.addAll(
+result(
+regClient.getWritableBookies()
+).getValue()
+);
+} else if (readonly) {
+bookies.addAll(
+result(
+regClient.getReadOnlyBookies()
+).getValue()
+);
+}
 
 Review comment:
   Is there any command to list all bookies and their state? For example, by 
default, we list all bookies running in the cluster and their running mode if 
no args is given. If we specify -ro, we only list read only bookies?


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] dlg99 commented on issue #1088: ISSUE #1086 (@bug W-4146427@) Client-side backpressure in netty (Fixes: io.netty.util.internal.OutOfDirectMemoryError under continuous heavy load)

[GitBox]

dlg99 commented on issue #1088: ISSUE #1086 (@bug W-4146427@) Client-side 
backpressure in netty (Fixes: io.netty.util.internal.OutOfDirectMemoryError 
under continuous heavy load)
URL: https://github.com/apache/bookkeeper/pull/1088#issuecomment-365740314
 
 
   @sijie sorry for the delay, missed the notification. 
   
   I re-implemented it with the following idea in mind:
   Do not block worker pool. we can write to netty channel even if it is not 
writable (as it happens without this change). When channel is not writable 
still write it to netty and send similar flag on PCBC and so on, bubbling up 
this status.
   LedgerHandle checks if it needs to slow down and waits until either time out 
expires or write allowed. 
   If timeout expires, LedgerHandle sets flag "allowFastFail" and still sends 
op downstream. in case allowFastFail is set and channel is still not writable 
PCBC fails op so ensemble change etc can happen.
   
   
   
   
   


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] jvrao commented on a change in pull request #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

jvrao commented on a change in pull request #1138: BP-31 BookKeeper Durability 
Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#discussion_r168311020
 
 

 ##
 File path: site/bps/BP-31-durability
 ##
 @@ -0,0 +1,134 @@
+
+---
+title: "BP-31: BookKeeper Durability(Anchor)"
+issue: https://github.com/apache/bookkeeper/BP-31-durability
+state: 'Anchor BP'
+release: "x.y.z"
+---
+## Motivation
+Apache BookKeeper is transitioning into a full fledged distributed storage 
that can keep the data for long term. Durability is paramount to achieve the 
status of trusted store. This Anchor BP discusses many gaps and areas of 
improvement.  Each issue listed here will have another issue and this BP is 
expected to be updated when that issue is created.
+
+## Durability Contract
+1. **Maintain WQ copies all the time**. If any ledger falls into under 
replicated state, there needs to be an SLA on how quickly the replication 
levels can be brought back to normal levels.
+2. **Enforce Placement Policy** strictly during write and replication.
+3. **Protect the data** against corruption on the wire or at rest.
+
+## Work Grouping (In the order of priority)
+### Detect Durability Validation
+First step is to understand the areas of durability breaches. Design metrics 
that record durability contract violations. 
+* At the Creation: Validate durability contract the extent is being created
+* At the Deletion: Validate durability contract when extent is deleted
+* During lifetime: Validate durability contract during the lifetime of the 
extent.(periodic validator)
+* During Read: IO or Checksum errors in the read path
+### Delete Discipline
+* Build a single delete choke point with stringent validations
+* Archival bit in the metadata to assist Two phase Deletes
+* Stateful/Explicit Deletes
+### Metadata Recovery
+* Metadata recovery tool to reconstruct the metadata if the metadata server 
gets wiped out. This tool need to make sure that the data is readable even if 
we can't get all the metadata (ex: ctime) back.
 
 Review comment:
   I just gave an example there. There can be more; The intention of the tool 
is to make ledgers readable. 


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] jvrao commented on a change in pull request #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

jvrao commented on a change in pull request #1138: BP-31 BookKeeper Durability 
Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#discussion_r168311642
 
 

 ##
 File path: site/bps/BP-31-durability
 ##
 @@ -0,0 +1,134 @@
+
+---
+title: "BP-31: BookKeeper Durability(Anchor)"
+issue: https://github.com/apache/bookkeeper/BP-31-durability
+state: 'Anchor BP'
+release: "x.y.z"
+---
+## Motivation
+Apache BookKeeper is transitioning into a full fledged distributed storage 
that can keep the data for long term. Durability is paramount to achieve the 
status of trusted store. This Anchor BP discusses many gaps and areas of 
improvement.  Each issue listed here will have another issue and this BP is 
expected to be updated when that issue is created.
+
+## Durability Contract
+1. **Maintain WQ copies all the time**. If any ledger falls into under 
replicated state, there needs to be an SLA on how quickly the replication 
levels can be brought back to normal levels.
+2. **Enforce Placement Policy** strictly during write and replication.
+3. **Protect the data** against corruption on the wire or at rest.
+
+## Work Grouping (In the order of priority)
+### Detect Durability Validation
+First step is to understand the areas of durability breaches. Design metrics 
that record durability contract violations. 
+* At the Creation: Validate durability contract the extent is being created
+* At the Deletion: Validate durability contract when extent is deleted
+* During lifetime: Validate durability contract during the lifetime of the 
extent.(periodic validator)
+* During Read: IO or Checksum errors in the read path
+### Delete Discipline
+* Build a single delete choke point with stringent validations
+* Archival bit in the metadata to assist Two phase Deletes
+* Stateful/Explicit Deletes
+### Metadata Recovery
+* Metadata recovery tool to reconstruct the metadata if the metadata server 
gets wiped out. This tool need to make sure that the data is readable even if 
we can't get all the metadata (ex: ctime) back.
+
+### Plug Durability Violations
+Our first step is to identify durability viloations. That gives us the 
magnitude of the problem and areas that we need to focus. In this phase, fix 
high impact areas.
+* Identify source of problems detected by the work we did in step-1 above 
(Detect Durability Validation)
+* Rereplicate under replicated extents detected during write
+* Rereplicate under replicated / corrupted extents detected during read
+* Replicated under replicated extents identified by periodic validator.
+### Durability Test
+* Test plan, new tests and integrating it into CI pipeline. 
+### Introduce bookie incarnation 
+* Design/Implement bookie incarnation mechanism 
+### End 2 End Checksum
+* Efficient checksum implementation (crc32c?)
 
 Review comment:
   crc32c is there, but I am not sure if we can't make any more changes to it. 
From @sijie 's numbers with crc32c is not a slamdunk. They are not great at 
lower payload size, even at 64k they are not negligible. So This is a BP will 
be to do more work in this area and come up with a method that has a negligible 
penalty.  


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] jvrao commented on issue #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

jvrao commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-365747490
 
 
   @sijie Not sure how to resolve Jenkin's failures for this Issue MD file. 


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

eolivelli commented on a change in pull request #1138: BP-31 BookKeeper 
Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#discussion_r168313262
 
 

 ##
 File path: site/bps/BP-31-durability
 ##
 @@ -0,0 +1,134 @@
+
+---
+title: "BP-31: BookKeeper Durability(Anchor)"
+issue: https://github.com/apache/bookkeeper/BP-31-durability
+state: 'Anchor BP'
+release: "x.y.z"
+---
+## Motivation
+Apache BookKeeper is transitioning into a full fledged distributed storage 
that can keep the data for long term. Durability is paramount to achieve the 
status of trusted store. This Anchor BP discusses many gaps and areas of 
improvement.  Each issue listed here will have another issue and this BP is 
expected to be updated when that issue is created.
+
+## Durability Contract
+1. **Maintain WQ copies all the time**. If any ledger falls into under 
replicated state, there needs to be an SLA on how quickly the replication 
levels can be brought back to normal levels.
+2. **Enforce Placement Policy** strictly during write and replication.
+3. **Protect the data** against corruption on the wire or at rest.
+
+## Work Grouping (In the order of priority)
+### Detect Durability Validation
+First step is to understand the areas of durability breaches. Design metrics 
that record durability contract violations. 
+* At the Creation: Validate durability contract the extent is being created
+* At the Deletion: Validate durability contract when extent is deleted
+* During lifetime: Validate durability contract during the lifetime of the 
extent.(periodic validator)
+* During Read: IO or Checksum errors in the read path
+### Delete Discipline
+* Build a single delete choke point with stringent validations
+* Archival bit in the metadata to assist Two phase Deletes
+* Stateful/Explicit Deletes
+### Metadata Recovery
+* Metadata recovery tool to reconstruct the metadata if the metadata server 
gets wiped out. This tool need to make sure that the data is readable even if 
we can't get all the metadata (ex: ctime) back.
 
 Review comment:
   I am citing custom metadata because they cannot be recreated from data on 
bookies actually and they are very useful at application layer (can be used to 
tell about the tenant which owns data for instance).
   Okay for leaving the phrase as it is now actually


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

eolivelli commented on a change in pull request #1138: BP-31 BookKeeper 
Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#discussion_r168313486
 
 

 ##
 File path: site/bps/BP-31-durability
 ##
 @@ -0,0 +1,134 @@
+
+---
+title: "BP-31: BookKeeper Durability(Anchor)"
+issue: https://github.com/apache/bookkeeper/BP-31-durability
+state: 'Anchor BP'
+release: "x.y.z"
+---
+## Motivation
+Apache BookKeeper is transitioning into a full fledged distributed storage 
that can keep the data for long term. Durability is paramount to achieve the 
status of trusted store. This Anchor BP discusses many gaps and areas of 
improvement.  Each issue listed here will have another issue and this BP is 
expected to be updated when that issue is created.
+
+## Durability Contract
+1. **Maintain WQ copies all the time**. If any ledger falls into under 
replicated state, there needs to be an SLA on how quickly the replication 
levels can be brought back to normal levels.
+2. **Enforce Placement Policy** strictly during write and replication.
+3. **Protect the data** against corruption on the wire or at rest.
+
+## Work Grouping (In the order of priority)
+### Detect Durability Validation
+First step is to understand the areas of durability breaches. Design metrics 
that record durability contract violations. 
+* At the Creation: Validate durability contract the extent is being created
+* At the Deletion: Validate durability contract when extent is deleted
+* During lifetime: Validate durability contract during the lifetime of the 
extent.(periodic validator)
+* During Read: IO or Checksum errors in the read path
+### Delete Discipline
+* Build a single delete choke point with stringent validations
+* Archival bit in the metadata to assist Two phase Deletes
+* Stateful/Explicit Deletes
+### Metadata Recovery
+* Metadata recovery tool to reconstruct the metadata if the metadata server 
gets wiped out. This tool need to make sure that the data is readable even if 
we can't get all the metadata (ex: ctime) back.
+
+### Plug Durability Violations
+Our first step is to identify durability viloations. That gives us the 
magnitude of the problem and areas that we need to focus. In this phase, fix 
high impact areas.
+* Identify source of problems detected by the work we did in step-1 above 
(Detect Durability Validation)
+* Rereplicate under replicated extents detected during write
+* Rereplicate under replicated / corrupted extents detected during read
+* Replicated under replicated extents identified by periodic validator.
+### Durability Test
+* Test plan, new tests and integrating it into CI pipeline. 
+### Introduce bookie incarnation 
+* Design/Implement bookie incarnation mechanism 
+### End 2 End Checksum
+* Efficient checksum implementation (crc32c?)
 
 Review comment:
   Makes sense to me


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

eolivelli commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-365749653
 
 
   Adding a 'Ignore ci' comment. This will enable merge of this patch in spite 
of ci failures


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

eolivelli commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-365749801
 
 
   Ignore CI


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 #1141: Replace DoubleByteBuf with ByteBufList

[GitBox]

eolivelli commented on issue #1141: Replace DoubleByteBuf with ByteBufList
URL: https://github.com/apache/bookkeeper/pull/1141#issuecomment-365750662
 
 
   Tests cannot compile due to a warning, can you fix it please? So that we can 
move forward


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] jvrao commented on issue #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

jvrao commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-365751061
 
 
   Thanks @eolivelli . Ignore CI comment on the pull request? If that is the 
process, I must have missed it. Doesn't BP make it implicit to the committer?. 


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] dlg99 commented on a change in pull request #1141: Replace DoubleByteBuf with ByteBufList

[GitBox]

dlg99 commented on a change in pull request #1141: Replace DoubleByteBuf with 
ByteBufList
URL: https://github.com/apache/bookkeeper/pull/1141#discussion_r168315873
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
 ##
 @@ -560,8 +562,7 @@ void addEntry(final long ledgerId, byte[] masterKey, final 
long entryId, ByteBuf
 .setOperation(OperationType.ADD_ENTRY)
 .setTxnId(txnId);
 
-byte[] toSendArray = new byte[toSend.readableBytes()];
-toSend.getBytes(toSend.readerIndex(), toSendArray);
+byte[] toSendArray = toSend.toArray();
 
 Review comment:
   ok, LGTM then.


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 #1130: handle zookeeper session expire in zk ledger manager

[GitBox]

eolivelli commented on a change in pull request #1130: handle zookeeper session 
expire in zk ledger manager
URL: https://github.com/apache/bookkeeper/pull/1130#discussion_r168317638
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
 ##
 @@ -409,7 +426,7 @@ public void processResult(int rc, String path, Object ctx, 
byte[] data, Stat sta
 public void writeLedgerMetadata(final long ledgerId, final LedgerMetadata 
metadata,
 final GenericCallback cb) {
 Version v = metadata.getVersion();
-if (Version.NEW == v || !(v instanceof LongVersion)) {
+if (!(v instanceof LongVersion)) {
 
 Review comment:
   @sijie do you mean that it is now possible for *v* to be '==' to Version.NEW 
because it cannot be of the same class so that the test is not useful?


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 #1130: handle zookeeper session expire in zk ledger manager

[GitBox]

eolivelli commented on a change in pull request #1130: handle zookeeper session 
expire in zk ledger manager
URL: https://github.com/apache/bookkeeper/pull/1130#discussion_r168317638
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
 ##
 @@ -409,7 +426,7 @@ public void processResult(int rc, String path, Object ctx, 
byte[] data, Stat sta
 public void writeLedgerMetadata(final long ledgerId, final LedgerMetadata 
metadata,
 final GenericCallback cb) {
 Version v = metadata.getVersion();
-if (Version.NEW == v || !(v instanceof LongVersion)) {
+if (!(v instanceof LongVersion)) {
 
 Review comment:
   @sijie do you mean that it is not possible for *v* to be '==' to Version.NEW 
because it cannot be of the same class so that the test is not useful?


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 #1088: ISSUE #1086 (@bug W-4146427@) Client-side backpressure in netty (Fixes: io.netty.util.internal.OutOfDirectMemoryError under continuous heavy load)

[GitBox]

sijie commented on issue #1088: ISSUE #1086 (@bug W-4146427@) Client-side 
backpressure in netty (Fixes: io.netty.util.internal.OutOfDirectMemoryError 
under continuous heavy load)
URL: https://github.com/apache/bookkeeper/pull/1088#issuecomment-365758418
 
 
   @dlg99 sgtm


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

eolivelli commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-365759111
 
 
   See https://github.com/apache/bookkeeper/pull/1145
   


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 #1130: handle zookeeper session expire in zk ledger manager

[GitBox]

sijie commented on a change in pull request #1130: handle zookeeper session 
expire in zk ledger manager
URL: https://github.com/apache/bookkeeper/pull/1130#discussion_r168329867
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
 ##
 @@ -409,7 +426,7 @@ public void processResult(int rc, String path, Object ctx, 
byte[] data, Stat sta
 public void writeLedgerMetadata(final long ledgerId, final LedgerMetadata 
metadata,
 final GenericCallback cb) {
 Version v = metadata.getVersion();
-if (Version.NEW == v || !(v instanceof LongVersion)) {
+if (!(v instanceof LongVersion)) {
 
 Review comment:
   @eolivelli I mean v can not be `Version.New` because line 416 expects a 
LongVersion. also the ledger manager api have separate api for 
CreateLedgerMetadata and WriteLedgerMetadata. so we don't expect Version.New 
and Version.ANY on `WriteLedgerMetadata`.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on issue #1133: Integration smoke test

[GitBox]

ivankelly commented on issue #1133: Integration smoke test
URL: https://github.com/apache/bookkeeper/pull/1133#issuecomment-365771167
 
 
   @sijie ok to merge this?


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


With regards,
Apache Git Services

[GitHub] ivankelly opened a new pull request #1146: Include yahoo custom version in all versions image

[GitBox]

ivankelly opened a new pull request #1146: Include yahoo custom version in all 
versions image
URL: https://github.com/apache/bookkeeper/pull/1146
 
 
   To ensure compatibility between versions during the merge, we need the
   yahoo custom version of bookkeeper.
   
   The most straightforward way to get this version is through
   pulsar. It's distributed as part of the pulsar distribution, with
   scripts to start it and all.
   


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


With regards,
Apache Git Services

[GitHub] 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-365772210
 
 
   > Is there any command to list all bookies and their state? For example, by 
default, we list all bookies running in the cluster and their running mode if 
no args is given. If we specify -ro, we only list read only bookies?
   
   Good point! I think the original change I made supports listing all bookies 
by default. I reverted it to limit to use either "-ro" or "-rw" only as the old 
CLI does per @reddycharan 's suggestion.
   
   @reddycharan do you think we should remove this limitation on new bookkeeper 
CLI to support "list all bookies" if no arg is provided or both args are 
provided.


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

[GitBox]

reddycharan commented on issue #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-365776717
 
 
   yeah, there is value in printing all bookies if no option is set. But in 
your earlier change you were printing all bookies for -rw option.


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-365779270
 
 
   @reddycharan yes. I fixed that after your first comment. your second comment 
was asking for printing bookies when "one and only one flag is set". so does it 
make sense for me to revert that? then the behavior will be:
   
   - "-rw" prints read/write bookies
   - "-ro" prints readonly bookies
   - no args prints both
   - "-rw" and "-ro" print both
   
   can we agree on this is the expected behavior?


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


With regards,
Apache Git Services

[GitHub] reddycharan commented on issue #1094: BP-27: New BookKeeper CLI

[GitBox]

reddycharan commented on issue #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-365780225
 
 
   I have an issue with "-rw" and "-ro" print both". In your logic if both the 
options are provided then you are printing both "-rw" and "-ro" bookies with no 
distinction. If you are distinguishing them with enough log messages ("List of 
ReadWrite bookies: .. List of ReadOnly bookies: ..") then ok


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-365781973
 
 
   @reddycharan okay I will update headlines like "readwrite bookies" and 
"readonly bookies". does that make sense?


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

[GitBox]

reddycharan commented on issue #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-365782689
 
 
   yes, with clear logging it should be good.


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 #1121: [Merge Yahoo repo]: AsyncReadLastEntry should trigger callback with error when ledger is empty

[GitBox]

sijie commented on a change in pull request #1121: [Merge Yahoo repo]: 
AsyncReadLastEntry should trigger callback with error when ledger is empty
URL: https://github.com/apache/bookkeeper/pull/1121#discussion_r168349715
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java
 ##
 @@ -178,4 +180,25 @@ public String toString() {
 protected void initializeExplicitLacFlushPolicy() {
 explicitLacFlushPolicy = 
ExplicitLacFlushPolicy.VOID_EXPLICITLAC_FLUSH_POLICY;
 }
+
+@Override
+public void asyncReadLastEntry(ReadCallback cb, Object ctx) {
+asyncReadLastConfirmed(new ReadLastConfirmedCallback() {
+@Override
+public void readLastConfirmedComplete(int rc, long lastConfirmed, 
Object ctx) {
+if (rc == BKException.Code.OK) {
+if (lastConfirmed < 0) {
+// Ledger was empty, so there is no last entry to read
+cb.readComplete(BKException.Code.NoSuchEntryException, 
ReadOnlyLedgerHandle.this, null, ctx);
+} else {
+asyncReadEntriesInternal(lastConfirmed, lastConfirmed, 
cb, ctx);
+}
+} else {
+LOG.error("ReadException in asyncReadLastEntry, ledgerId: 
{}, lac: {}, rc:{}",
+lastConfirmed, ledgerId, rc);
+cb.readComplete(BKException.Code.ReadException, 
ReadOnlyLedgerHandle.this, null, ctx);
 
 Review comment:
   cb.readCpomplete(rc, ...)
   
   you can pass the rc through to the application.


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 #1121: [Merge Yahoo repo]: AsyncReadLastEntry should trigger callback with error when ledger is empty

[GitBox]

sijie commented on a change in pull request #1121: [Merge Yahoo repo]: 
AsyncReadLastEntry should trigger callback with error when ledger is empty
URL: https://github.com/apache/bookkeeper/pull/1121#discussion_r168349626
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##
 @@ -787,6 +788,35 @@ public void onFailure(Throwable cause) {
 }
 }
 
+/*
+ * Read the last entry in the ledger
+ *
+ * @param cb
+ *object implementing read callback interface
+ * @param ctx
+ *control object
+ */
+public void asyncReadLastEntry(ReadCallback cb, Object ctx) {
+long lastEntryId = getLastAddConfirmed();
+if (lastEntryId < 0) {
+// Ledger was empty, so there is no last entry to read
+cb.readComplete(BKException.Code.NoSuchEntryException, this, null, 
ctx);
+} else {
+asyncReadEntriesInternal(lastEntryId, lastEntryId, cb, ctx);
+}
+}
+
+public org.apache.bookkeeper.client.api.LedgerEntry readLastEntry()
+throws InterruptedException, ExecutionException, BKException {
 
 Review comment:
   we should not throw "ExcuctionException" for sync methods. keep the 
behaviour same as other sync methods.
   
   you can check 
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java#L594
 as an example.
   
   you can use SyncReadCallback as other methods are using.


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168349884
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
 
 Review comment:
   any reason for keeping it in bookkeeper/bin folder instead of 
bookkeeper/bookkeeper-server/bin ?


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168351196
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
+#
+#/**
+# * Copyright 2007 The Apache Software Foundation
+# *
+# * 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.
+# */
+
+BINDIR=`dirname "$0"`
+BK_HOME=`cd $BINDIR/..;pwd`
+
+DEFAULT_CONF=$BK_HOME/conf/bk_server.conf
 
 Review comment:
   here $BK_HOME would be 'bookkeeper' folder but not 
'bookkeeper/bookkeeper-server' folder


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168351196
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
+#
+#/**
+# * Copyright 2007 The Apache Software Foundation
+# *
+# * 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.
+# */
+
+BINDIR=`dirname "$0"`
+BK_HOME=`cd $BINDIR/..;pwd`
+
+DEFAULT_CONF=$BK_HOME/conf/bk_server.conf
 
 Review comment:
   here $BK_HOME would be 'bookkeeper' folder but not 
'bookkeeper/bookkeeper-server' folder, so it wouldn't work


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168351196
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
+#
+#/**
+# * Copyright 2007 The Apache Software Foundation
+# *
+# * 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.
+# */
+
+BINDIR=`dirname "$0"`
+BK_HOME=`cd $BINDIR/..;pwd`
+
+DEFAULT_CONF=$BK_HOME/conf/bk_server.conf
 
 Review comment:
   here $BK_HOME would be 'bookkeeper' folder but not 
'bookkeeper/bookkeeper-server' folder, so it wouldn't work


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168351975
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
 
 Review comment:
   the thought here is eventually moving bin/ conf/ directories to the root 
module. since we will split bookkeeper-server into client and server module in 
future, and bookkeeper-dist is for packing the binary distribution. moving bin 
to top-level will make things easier and manageable. I am doing this for this 
new bookkeeper CLI first and will move other script after we split 
bookkeeper-server module in 4.8.


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168352326
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
 
 Review comment:
   to un this shell script, do I have to config / set anything, I'm getting 
following error
   
   /Users/cguttapalem/Workspace/Community/bookkeeper/bin
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli 
   JAVA_HOME not set, using java from PATH. (/usr/bin/java)
   Fail to load sub command 'cluster' : null
   java.lang.reflect.InvocationTargetException
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at 
sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at 
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
at 
org.apache.bookkeeper.tools.cli.BookieShell.newCommandInstance(BookieShell.java:69)
at 
org.apache.bookkeeper.tools.cli.BookieShell.setupShell(BookieShell.java:122)
at org.apache.bookkeeper.tools.cli.BookieShell.run(BookieShell.java:142)
at 
org.apache.bookkeeper.tools.cli.BookieShell.main(BookieShell.java:205)
   Caused by: java.lang.NoClassDefFoundError: 
org/apache/bookkeeper/stats/StatsLogger
at 
org.apache.bookkeeper.tools.cli.commands.CmdCluster.(CmdCluster.java:32)
... 8 more
   Caused by: java.lang.ClassNotFoundException: 
org.apache.bookkeeper.stats.StatsLogger
at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
... 9 more
   


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-365788909
 
 
   @reddycharan I updated this PR to make listbookies to list all bookies if no 
args is provided or both args are provided.


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-365788948
 
 
   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] reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper CLI

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168354161
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
+#
+#/**
+# * Copyright 2007 The Apache Software Foundation
+# *
+# * 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.
+# */
+
+BINDIR=`dirname "$0"`
+BK_HOME=`cd $BINDIR/..;pwd`
+
+DEFAULT_CONF=$BK_HOME/conf/bk_server.conf
 
 Review comment:
   in bookkeeper/conf folder there isn't bk_server.conf, it is in 
bookkeeper/bookkeeper-server/conf folder
   
   so getting following error -
   
   Failed to load configuration file 
'/Users/cguttapalem/Workspace/Community/bookkeeper/conf/bk_server.conf' : 
Unable to load the configuration from the URL 
file:/Users/cguttapalem/Workspace/Community/bookkeeper/conf/bk_server.conf


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168355576
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
 
 Review comment:
   @sijie after rebasing changes and rebuilding, this error is not reproing 
now, wondering in which commit you fixed 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] reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper CLI

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168356754
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli bookie
   
   Usage: bookie-shell bookie [options] [command] [command options]
 Commands:
   lastmark  Print last log marker
 Usage: lastmark [options]
   
   
   
   in the last line of the output, there shouldn't be "[options]" in "Usage" 
line, since there are no options for "lastmark" command


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

[GitBox]

athanatos commented on issue #970: ISSUE #966: Expose quorum write complete 
latency to the client
URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-365794149
 
 
   @eolivelli repushed with fix


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 #1121: [Merge Yahoo repo]: AsyncReadLastEntry should trigger callback with error when ledger is empty

[GitBox]

sijie commented on issue #1121: [Merge Yahoo repo]: AsyncReadLastEntry should 
trigger callback with error when ledger is empty
URL: https://github.com/apache/bookkeeper/pull/1121#issuecomment-365796214
 
 
   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 #1121: [Merge Yahoo repo]: AsyncReadLastEntry should trigger callback with error when ledger is empty

[GitBox]

sijie commented on issue #1121: [Merge Yahoo repo]: AsyncReadLastEntry should 
trigger callback with error when ledger is empty
URL: https://github.com/apache/bookkeeper/pull/1121#issuecomment-365796195
 
 
   @jvrao can you review this PR again since you were involved in this PR?


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168359420
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
 
 Review comment:
   do you run with a clean build after you switch the branch?


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168359769
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
+#
+#/**
+# * Copyright 2007 The Apache Software Foundation
+# *
+# * 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.
+# */
+
+BINDIR=`dirname "$0"`
+BK_HOME=`cd $BINDIR/..;pwd`
+
+DEFAULT_CONF=$BK_HOME/conf/bk_server.conf
 
 Review comment:
   yes. that is done intentionally at this PR. was planning to move the bin/ 
and conf/ to root directory after we complete the new bookkeeper CLI. otherwise 
we will have two bk_server.conf in place, which make things a bit hard to keep 
it in sync.
   
   for testing purpose, you can manually copy 
bookkeeper-server/conf/bk_server.conf to conf.
   
   If you feel strong that we need to copy 
bookkeeper-server/conf/bk_server.conf  right now, we don't have to wait until 
new bookkeeper cli is completed, I am fine to copy the file as well.


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168359983
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   @reddycharan this output is managed by jcommander. I don't think it is easy 
to get round at this moment. is this a real concern?


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


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #1147: Move bookkeeper-server/{bin,conf} to root module

[GitBox]

sijie opened a new issue #1147: Move bookkeeper-server/{bin,conf} to root module
URL: https://github.com/apache/bookkeeper/issues/1147
 
 
   since 4.7, we are shipping the binary distribution using bookkeeper-dist 
modules. so it is making more sense to have {bin,conf} directory at root 
module, rather than bookkeeper-server module. This is also a change for 
bookkeeper new cli


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168361120
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
+#
+#/**
+# * Copyright 2007 The Apache Software Foundation
+# *
+# * 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.
+# */
+
+BINDIR=`dirname "$0"`
+BK_HOME=`cd $BINDIR/..;pwd`
+
+DEFAULT_CONF=$BK_HOME/conf/bk_server.conf
 
 Review comment:
   created an issue for #1147 and change the default conf to 
bookkeeper-server/conf/bk_server.conf, so this makes things clear.
   
   #1147 will be the blocker for shipping new bookkeeper CLI in the binary 
distribution. because this script won't work in binary distribution.


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-365798669
 
 
   @reddycharan I have addressed your comments. please take a look again.


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

sijie commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-36573
 
 
   @eolivelli in general, I think we should use "Ignore CI" just before a 
committer is about to merge it, and it should be the committer who is merging 
the PR do this. otherwise, the comments will become unmanageable. For a BP 
change, it is probably okay. For other code changes, there might be other 
review comments and changes coming after you put "Ignore CI". Then the "Ignore 
CI" might potentially ignores those changes coming after your comment which are 
problematic. 
   
   so in general, I would suggest the committer who is merging a PR, check the 
CI results before he merges it and leave "Ignore CI" after he confirms the CI 
build failures come from flakes.


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

sijie commented on issue #1138: BP-31 BookKeeper Durability Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#issuecomment-365800410
 
 
   @jvrao the jenkins CI failed because changing default ledger manager class 
from flat to hierarchical. #1137 has fixed this. in this case, you can comment 
"retest this please" in this PR. It will automatically trigger jenkins job to 
run.


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 #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

sijie commented on a change in pull request #1138: BP-31 BookKeeper Durability 
Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#discussion_r168363317
 
 

 ##
 File path: site/bps/BP-31-durability
 ##
 @@ -0,0 +1,134 @@
+
+---
+title: "BP-31: BookKeeper Durability(Anchor)"
+issue: https://github.com/apache/bookkeeper/BP-31-durability
+state: 'Anchor BP'
+release: "x.y.z"
+---
+## Motivation
+Apache BookKeeper is transitioning into a full fledged distributed storage 
that can keep the data for long term. Durability is paramount to achieve the 
status of trusted store. This Anchor BP discusses many gaps and areas of 
improvement.  Each issue listed here will have another issue and this BP is 
expected to be updated when that issue is created.
+
+## Durability Contract
+1. **Maintain WQ copies all the time**. If any ledger falls into under 
replicated state, there needs to be an SLA on how quickly the replication 
levels can be brought back to normal levels.
+2. **Enforce Placement Policy** strictly during write and replication.
+3. **Protect the data** against corruption on the wire or at rest.
+
+## Work Grouping (In the order of priority)
+### Detect Durability Validation
+First step is to understand the areas of durability breaches. Design metrics 
that record durability contract violations. 
+* At the Creation: Validate durability contract the extent is being created
+* At the Deletion: Validate durability contract when extent is deleted
+* During lifetime: Validate durability contract during the lifetime of the 
extent.(periodic validator)
+* During Read: IO or Checksum errors in the read path
+### Delete Discipline
+* Build a single delete choke point with stringent validations
+* Archival bit in the metadata to assist Two phase Deletes
+* Stateful/Explicit Deletes
+### Metadata Recovery
+* Metadata recovery tool to reconstruct the metadata if the metadata server 
gets wiped out. This tool need to make sure that the data is readable even if 
we can't get all the metadata (ex: ctime) back.
+
+### Plug Durability Violations
+Our first step is to identify durability viloations. That gives us the 
magnitude of the problem and areas that we need to focus. In this phase, fix 
high impact areas.
+* Identify source of problems detected by the work we did in step-1 above 
(Detect Durability Validation)
+* Rereplicate under replicated extents detected during write
+* Rereplicate under replicated / corrupted extents detected during read
+* Replicated under replicated extents identified by periodic validator.
+### Durability Test
+* Test plan, new tests and integrating it into CI pipeline. 
+### Introduce bookie incarnation 
+* Design/Implement bookie incarnation mechanism 
+### End 2 End Checksum
+* Efficient checksum implementation (crc32c?)
+* Implement checksum validation on bookies in the write path. 
+### Soft Deletes
+* Design and implement soft delete feature
+### BitRot detection
+* Design and implement bitrot detection/correction.
+
+## Durability Contract Violations 
+### Write errors beyond AQ are ignored.
+BK client library transparently corrects any write errors while writing to 
bookie by changing the ensemble. Take a case where `WQ:3 and AQ:2`. This works 
fine only if the write fails to the bookie before it gets 2 successful 
responses. But if the 3rd bookie write fails **after** 2 successful responses 
and the response sent to client, this error is logged and no immediate action 
is taken to bring up the replication of the entry.
+This case **may not be**  detected by the auditor?s periodic ledger check. 
Given that we allow out of order write, that in the combination of 2 out of 3 
to satisfy client, it is possible to have under replication in the middle of 
the ensemble entry. Hence ledgercheck is not going to find all under 
replication cases, on top of that,   periodic ledger check  is a complete sweep 
of the store, an very expensive and slow crawl hence defaulted to once a week 
run.
+
+### Strict enforcement of placement policy 
+The best effort placement policy increases the write availability but at the 
cost of durability. Due to this non-strict placement, BK can?t guarantee data 
availability when a fault domain (rack) is lost. This also makes rolling 
upgrade across fault domains more difficult/non possible. Need to enforce 
strict ensemble placement and fail the write if all WQ copies are not able to 
be placed across different fault domains.  Minor fix/enhancement if we agree to 
give placement higher priority than a successful write(availability)
+
+The auditor re-replication uses client library to find a replacement bookie 
for each ledger in the lost bookie. But bookies are unaware of the ledger 
ensemble placement policy as this information is not part of metadata. 
 
 Review comment:
   The placement policy base re-replication is already there.
   
   I think this change here is more about making placement policy as part of 
metadata if I understand t

[GitHub] sijie commented on a change in pull request #1138: BP-31 BookKeeper Durability Anchor

[GitBox]

sijie commented on a change in pull request #1138: BP-31 BookKeeper Durability 
Anchor
URL: https://github.com/apache/bookkeeper/pull/1138#discussion_r168363075
 
 

 ##
 File path: site/bps/BP-31-durability
 ##
 @@ -0,0 +1,134 @@
+
+---
+title: "BP-31: BookKeeper Durability(Anchor)"
+issue: https://github.com/apache/bookkeeper/BP-31-durability
+state: 'Anchor BP'
+release: "x.y.z"
+---
+## Motivation
+Apache BookKeeper is transitioning into a full fledged distributed storage 
that can keep the data for long term. Durability is paramount to achieve the 
status of trusted store. This Anchor BP discusses many gaps and areas of 
improvement.  Each issue listed here will have another issue and this BP is 
expected to be updated when that issue is created.
+
+## Durability Contract
+1. **Maintain WQ copies all the time**. If any ledger falls into under 
replicated state, there needs to be an SLA on how quickly the replication 
levels can be brought back to normal levels.
+2. **Enforce Placement Policy** strictly during write and replication.
+3. **Protect the data** against corruption on the wire or at rest.
+
+## Work Grouping (In the order of priority)
+### Detect Durability Validation
+First step is to understand the areas of durability breaches. Design metrics 
that record durability contract violations. 
+* At the Creation: Validate durability contract the extent is being created
+* At the Deletion: Validate durability contract when extent is deleted
+* During lifetime: Validate durability contract during the lifetime of the 
extent.(periodic validator)
+* During Read: IO or Checksum errors in the read path
+### Delete Discipline
+* Build a single delete choke point with stringent validations
+* Archival bit in the metadata to assist Two phase Deletes
+* Stateful/Explicit Deletes
+### Metadata Recovery
+* Metadata recovery tool to reconstruct the metadata if the metadata server 
gets wiped out. This tool need to make sure that the data is readable even if 
we can't get all the metadata (ex: ctime) back.
+
+### Plug Durability Violations
+Our first step is to identify durability viloations. That gives us the 
magnitude of the problem and areas that we need to focus. In this phase, fix 
high impact areas.
+* Identify source of problems detected by the work we did in step-1 above 
(Detect Durability Validation)
+* Rereplicate under replicated extents detected during write
 
 Review comment:
   would be good to change "extents" to "ledgers" :-)


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168364008
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   
   
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli 
   
   Usage: bookie-shell [options] [command] [command options]
 Options:
   -c, --conf
  Bookie Configuration File
   -h, --help
  Show this help message
  Default: false
 Commands:
   bookie  Commands on operating a single bookie
 Usage: bookie [options]
   
   client  Commands that interact with a cluster
 Usage: client [options]
   
   cluster  Commands that operate a cluster
 Usage: cluster [options]
   
   metadata  Commands that interact with metadata storage
 Usage: metadata [options]
   
   
   1) it is definitely not obvious for the first time users
   
   2) In the following line what does "bookie-shell" refer to? because when I 
tried "./bookkeeper-cli bookie-shell bookie" it doesn't output just "bookie" 
command  usage.
   
   Usage: bookie-shell [options] [command] [command options]
   
   3) in the existing shell we have couple of options for commands - 
ledgeridformat and entryformat. It seems it is missing in the new CLI
   
   4) I'm concerned about flexibility with jcommander framework for commands 
with complicated options like 'readlog' command - (readlog  [-msg] 
 [-ledgerid  [-entryid 
]] [-startpos  [-endpos ]])


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168364008
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli 
   
   Usage: bookie-shell [options] [command] [command options]
 Options:
   -c, --conf
  Bookie Configuration File
   -h, --help
  Show this help message
  Default: false
   
 Commands:
   
   bookie  Commands on operating a single bookie
 Usage: bookie [options]
   
   client  Commands that interact with a cluster
 Usage: client [options]
   
   cluster  Commands that operate a cluster
 Usage: cluster [options]
   
   metadata  Commands that interact with metadata storage
 Usage: metadata [options]
   
   
   1) it is definitely not obvious for the first time users
   
   2) In the following line what does "bookie-shell" refer to? because when I 
tried "./bookkeeper-cli bookie-shell bookie" it doesn't output just "bookie" 
command  usage.
   
   Usage: bookie-shell [options] [command] [command options]
   
   3) in the existing shell we have couple of options for commands - 
ledgeridformat and entryformat. It seems it is missing in the new CLI
   
   4) I'm concerned about flexibility with jcommander framework for commands 
with complicated options like 'readlog' command - (readlog  [-msg] 
 [-ledgerid  [-entryid 
]] [-startpos  [-endpos ]])


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168364631
 
 

 ##
 File path: bin/bookkeeper-cli
 ##
 @@ -0,0 +1,153 @@
+#!/usr/bin/env bash
 
 Review comment:
   probably not..and yeah that could be reason for why it didnt repro without 
any code 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] reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper CLI

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168366800
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   5. What is the expected output for the command "./bookkeeper-cli --help 
bookie" ? isn't it supposed to print help message just for bookie command?
   
   It is printing the full output of bookkeeper-cli
   
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli --help bookie

   Usage: bookie-shell [options] [command] [command options]
 Options:
   -c, --conf
  Bookie Configuration File
   -h, --help
  Show this help message
  Default: false
 Commands:
   bookie  Commands on operating a single bookie
 Usage: bookie [options]
   
   client  Commands that interact with a cluster
 Usage: client [options]
   
   cluster  Commands that operate a cluster
 Usage: cluster [options]
   
   metadata  Commands that interact with metadata storage
 Usage: metadata [options]
   
   
   


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168366800
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   5. What is the expected output for the command "./bookkeeper-cli --help 
bookie" ? isn't it supposed to print help message just for bookie command?
   
   It is printing the full output of bookkeeper-cli
   
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli --help bookie

   Usage: bookie-shell [options] [command] [command options]
   
 Options:
   
   -c, --conf
  Bookie Configuration File
   -h, --help
  Show this help message
  Default: false
   
 Commands:
   
   bookie  Commands on operating a single bookie
 Usage: bookie [options]
   
   client  Commands that interact with a cluster
 Usage: client [options]
   
   cluster  Commands that operate a cluster
 Usage: cluster [options]
   
   metadata  Commands that interact with metadata storage
 Usage: metadata [options]
   
   
   


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

[GitBox]

reddycharan commented on a change in pull request #1094: BP-27: New BookKeeper 
CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168367675
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   6) in the output for the following command - what does "[options]" referred 
to in the following line
   
   "Usage: bookie-shell client [options] [command] [command options]"
   
   cguttapalem@cguttapale-ltm1:~/Workspace/Community/bookkeeper/bin$ 
./bookkeeper-cli client
   JAVA_HOME not set, using java from PATH. (/usr/bin/java)
   Usage: bookie-shell client [options] [command] [command options]
 Commands:
   simpletest  Simple test to create a ledger and write entries to it.
 Usage: simpletest [options]
   Options:
 -a, --ack-quorum-size
Ack quorum size (default 2)
Default: 2
 -e, --ensemble-size
Ensemble size (default 3)
Default: 3
 -n, --num-entries
Entries to write (default 100)
Default: 100
 -w, --write-quorum-size
Write quorum size (default 2)
Default: 2
   
   
   


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

[GitBox]

sijie commented on a change in pull request #1094: BP-27: New BookKeeper CLI 
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168367655
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   > 1. it is definitely not obvious for the first time users
   
   users usually have a period to get used to a cli. is this a real concern?
   
   > 2. In the following line what does "bookie-shell" refer to? because when I 
tried "./bookkeeper-cli bookie-shell bookie" it doesn't output just "bookie" 
command usage.
   
   will change it to "bookkeeper-cli". is that clear to you?
   
   > 3. in the existing shell we have couple of options for commands - 
ledgeridformat and entryformat. It seems it is missing in the new CLI
   
   if you check the description, this is the first change for BP-27. I am 
setting up the skeleton and only port 3 commands, there will be subsequent 
changes to port individual commands.
   
   > 4. I'm concerned about flexibility with jcommander framework for commands 
with complicated options like 'readlog' command - (readlog [-msg]  [-ledgerid [-entryid ]] [-startpos [-endpos ]])
   
   why? jcommander is just a parser like commons-cli.
   
   if you ship this PR, I can port the `readlog` command in the next PR. 
otherwise we will keep stuck here and will never move.
   
   > 5. What is the expected output for the command "./bookkeeper-cli --help 
bookie" ? isn't it supposed to print help message just for bookie command?
   
   --help before "bookie" is printing the help usage of "./bookkeeper-cli". I 
will have a subsequent change for supporting './bookkeeper-cli help 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 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-365807809
 
 
   @reddycharan 
   
   I'd like to clarify this PR is the first change for BP-27, it just tries to 
setup the skeleton and port only 3 commands. more changes are coming if we 
agree on the skeleton. also this change is a new CLI, it doesn't change 
existing behavior of old bookie shell and break any BC. This PR doesn't target 
on replacing everything of BookieShell.


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 #1094: BP-27 (part 1): New BookKeeper CLI Skeleton

[GitBox]

reddycharan commented on issue #1094: BP-27 (part 1): New BookKeeper CLI 
Skeleton
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-365808246
 
 
   @sijie  sounds good..I'll wrap up this CR by the end of today. For sure by 
tomorrow I'll give my +1


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


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1094: BP-27 (part 1): New BookKeeper CLI Skeleton

[GitBox]

sijie commented on issue #1094: BP-27 (part 1): New BookKeeper CLI Skeleton
URL: https://github.com/apache/bookkeeper/pull/1094#issuecomment-365808654
 
 
   @reddycharan updated this PR to use "bookkeeper-cli" instead of 
"bookie-shell" in the command output, to avoid confusion.


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 #1094: BP-27 (part 1): New BookKeeper CLI Skeleton

[GitBox]

sijie commented on a change in pull request #1094: BP-27 (part 1): New 
BookKeeper CLI Skeleton
URL: https://github.com/apache/bookkeeper/pull/1094#discussion_r168369025
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookie/LastMarkCommand.java
 ##
 @@ -0,0 +1,61 @@
+/*
+ * 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.tools.cli.commands.bookie;
+
+import com.beust.jcommander.Parameters;
+import com.google.common.collect.Lists;
+import java.util.List;
+import org.apache.bookkeeper.bookie.Journal;
+import org.apache.bookkeeper.bookie.LedgerDirsManager;
+import org.apache.bookkeeper.bookie.LogMark;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.util.DiskChecker;
+
+/**
+ * A bookie command to print the last log marker.
+ */
+@Parameters(commandDescription = "Print last log marker")
+public class LastMarkCommand extends BookieCommand {
 
 Review comment:
   "bookie-shell client [options] [command] [command options]" this is 
formatted by jcommander.

   for a given command, it has options (seen as "[options]") and then 
sub-commands ("[command]"). so here "[options]" is the options of "client" 
command. but there is no "options" defined for "client" command, so it is not 
printed in the output. it only prints the "commands" for "client" command: 
"simpletest"
   
   


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] jvrao commented on a change in pull request #1088: ISSUE #1086 (@bug W-4146427@) Client-side backpressure in netty (Fixes: io.netty.util.internal.OutOfDirectMemoryError under continuous heavy

[GitBox]

jvrao commented on a change in pull request #1088: ISSUE #1086 (@bug 
W-4146427@) Client-side backpressure in netty (Fixes: 
io.netty.util.internal.OutOfDirectMemoryError under continuous heavy load)
URL: https://github.com/apache/bookkeeper/pull/1088#discussion_r168384166
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
 ##
 @@ -150,6 +150,16 @@ private int getRc(int rc) {
 return faultyBookies;
 }
 
+public boolean isWritable(BookieSocketAddress address) {
+final PerChannelBookieClientPool pcbcPool = lookupClient(address);
+if (pcbcPool == null) {
+// let write fail with whatever error it produces
+return true;
+}
+
+return pcbcPool.isWritable(0L /*not really used in obtain()*/);
 
 Review comment:
   If we have multiple channels in the pool you are checking only one here 
isn't 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] jvrao commented on a change in pull request #1088: ISSUE #1086 (@bug W-4146427@) Client-side backpressure in netty (Fixes: io.netty.util.internal.OutOfDirectMemoryError under continuous heavy

[GitBox]

jvrao commented on a change in pull request #1088: ISSUE #1086 (@bug 
W-4146427@) Client-side backpressure in netty (Fixes: 
io.netty.util.internal.OutOfDirectMemoryError under continuous heavy load)
URL: https://github.com/apache/bookkeeper/pull/1088#discussion_r168380400
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##
 @@ -197,6 +207,7 @@ public long getBookiePendingRequests(BookieSocketAddress 
bookieSocketAddress) {
 ensembleChangeCounter = 
bk.getStatsLogger().getCounter(BookKeeperClientStats.ENSEMBLE_CHANGES);
 lacUpdateHitsCounter = 
bk.getStatsLogger().getCounter(BookKeeperClientStats.LAC_UPDATE_HITS);
 lacUpdateMissesCounter = 
bk.getStatsLogger().getCounter(BookKeeperClientStats.LAC_UPDATE_MISSES);
+sendWaitTimer = 
bk.getStatsLogger().getOpStatsLogger(BookKeeperClientStats.CLIENT_SEND_WAIT_TIMER);
 
 Review comment:
   Without Counter suffix, it might get confused to be a timer than a stat


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