[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r146483992
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -797,6 +763,10 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
 return;
 }
 
+if (dryrun) {
+System.out.println("Recovered ledger " + lId + " : " + 
(fenceRequired ? "[fence required]" : ""));
 
 Review comment:
   Can't it be separated out using a different Logger object?


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r146483788
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -739,19 +684,40 @@ public void openComplete(int rc, final LedgerHandle lh, 
Object ctx) {
 asyncOpenLedger(lId, new OpenCallback() {
 @Override
 public void openComplete(int newrc, final 
LedgerHandle newlh, Object newctx) {
-if (newrc != Code.OK.intValue()) {
+if (newrc != BKException.Code.OK) {
 LOG.error("BK error close ledger: " + lId, 
BKException.create(newrc));
-ledgerIterCb.processResult(newrc, null, 
null);
+finalLedgerIterCb.processResult(newrc, 
null, null);
 return;
 }
-// do recovery
-recoverLedger(bookieSrc, lId, ledgerIterCb, 
availableBookies);
+bkc.mainWorkerPool.submit(() -> {
+// do recovery
+recoverLedger(bookiesSrc, lId, dryrun, 
skipOpenLedgers, finalLedgerIterCb);
+});
 }
 }, null);
 return;
 }
 }
 
+final AsyncCallback.VoidCallback ledgerIterCb = new 
AsyncCallback.VoidCallback() {
 
 Review comment:
   no problem with 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] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r146483553
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -672,55 +619,52 @@ public void process(Long ledgerId, 
AsyncCallback.VoidCallback iterCallback) {
 }
 
 /**
- * Get a new random bookie, but ensure that it isn't one that is already
- * in the ensemble for the ledger.
- */
-private BookieSocketAddress getNewBookie(final List 
bookiesAlreadyInEnsemble,
-final List availableBookies)
-throws BKException.BKNotEnoughBookiesException {
-ArrayList candidates = new 
ArrayList();
-candidates.addAll(availableBookies);
-candidates.removeAll(bookiesAlreadyInEnsemble);
-if (candidates.size() == 0) {
-throw new BKException.BKNotEnoughBookiesException();
-}
-return candidates.get(rand.nextInt(candidates.size()));
-}
-
-/**
  * This method asynchronously recovers a given ledger if any of the ledger
  * entries were stored on the failed bookie.
  *
- * @param bookieSrc
- *Source bookie that had a failure. We want to replicate the
+ * @param bookiesSrc
+ *Source bookies that had a failure. We want to replicate the
  *ledger fragments that were stored there.
  * @param lId
  *Ledger id we want to recover.
- * @param ledgerIterCb
+ * @param dryrun
+ *printing the recovery plan without actually recovering 
bookies
+ * @param skipOpenLedgers
+ *Skip recovering open ledgers.
+ * @param finalLedgerIterCb
  *IterationCallback to invoke once we've recovered the current
  *ledger.
- * @param availableBookies
- *List of Bookie Servers that are available to use for
- *replicating data on the failed bookie. This could contain a
- *single bookie server if the user explicitly chose a bookie
- *server to replicate data to.
  */
-private void recoverLedger(final BookieSocketAddress bookieSrc, final long 
lId,
-final AsyncCallback.VoidCallback ledgerIterCb, final 
List availableBookies) {
+private void recoverLedger(final Set bookiesSrc, 
final long lId, final boolean dryrun,
+   final boolean skipOpenLedgers, final 
AsyncCallback.VoidCallback finalLedgerIterCb) {
 if (LOG.isDebugEnabled()) {
 LOG.debug("Recovering ledger : {}", lId);
 }
 
 asyncOpenLedgerNoRecovery(lId, new OpenCallback() {
 @Override
 public void openComplete(int rc, final LedgerHandle lh, Object 
ctx) {
-if (rc != Code.OK.intValue()) {
+if (rc != BKException.Code.OK) {
 LOG.error("BK error opening ledger: " + lId, 
BKException.create(rc));
-ledgerIterCb.processResult(rc, null, null);
+finalLedgerIterCb.processResult(rc, null, null);
 return;
 }
 
 LedgerMetadata lm = lh.getLedgerMetadata();
+if (skipOpenLedgers && !lm.isClosed() && !lm.isInRecovery()) {
+LOG.info("Skip recovering open ledger {}.", lId);
+try {
+lh.close();
+} catch (InterruptedException ie) {
+Thread.currentThread().interrupt();
+} catch (BKException bke) {
+LOG.warn("Error on closing ledger handle for {}.", 
lId);
+}
+finalLedgerIterCb.processResult(BKException.Code.OK, null, 
null);
+return;
+}
+
+boolean fenceRequired = false;
 
 Review comment:
   actually, in that case i'd do
   final boolean fenceRequired = !lm.isClosed() && 
containBookies(lm.getEnsembles().lastEntry(), bookieSrc);
   
   What I want to avoid is setting fenceRequired more than once. The logic here 
is complex. When reading the code you need to check what a value should be set 
to, and this is harder if it is set in multiple places.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r146476663
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -739,19 +684,40 @@ public void openComplete(int rc, final LedgerHandle lh, 
Object ctx) {
 asyncOpenLedger(lId, new OpenCallback() {
 @Override
 public void openComplete(int newrc, final 
LedgerHandle newlh, Object newctx) {
-if (newrc != Code.OK.intValue()) {
+if (newrc != BKException.Code.OK) {
 LOG.error("BK error close ledger: " + lId, 
BKException.create(newrc));
-ledgerIterCb.processResult(newrc, null, 
null);
+finalLedgerIterCb.processResult(newrc, 
null, null);
 return;
 }
-// do recovery
-recoverLedger(bookieSrc, lId, ledgerIterCb, 
availableBookies);
+bkc.mainWorkerPool.submit(() -> {
+// do recovery
+recoverLedger(bookiesSrc, lId, dryrun, 
skipOpenLedgers, finalLedgerIterCb);
+});
 }
 }, null);
 return;
 }
 }
 
+final AsyncCallback.VoidCallback ledgerIterCb = new 
AsyncCallback.VoidCallback() {
 
 Review comment:
   The reason I suggested the utility message is that when I came to this in 
the review, I was like "why is he adding this wrapper here". The logging within 
the callback doesn't clarify why.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r146476218
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -672,55 +619,52 @@ public void process(Long ledgerId, 
AsyncCallback.VoidCallback iterCallback) {
 }
 
 /**
- * Get a new random bookie, but ensure that it isn't one that is already
- * in the ensemble for the ledger.
- */
-private BookieSocketAddress getNewBookie(final List 
bookiesAlreadyInEnsemble,
-final List availableBookies)
-throws BKException.BKNotEnoughBookiesException {
-ArrayList candidates = new 
ArrayList();
-candidates.addAll(availableBookies);
-candidates.removeAll(bookiesAlreadyInEnsemble);
-if (candidates.size() == 0) {
-throw new BKException.BKNotEnoughBookiesException();
-}
-return candidates.get(rand.nextInt(candidates.size()));
-}
-
-/**
  * This method asynchronously recovers a given ledger if any of the ledger
  * entries were stored on the failed bookie.
  *
- * @param bookieSrc
- *Source bookie that had a failure. We want to replicate the
+ * @param bookiesSrc
+ *Source bookies that had a failure. We want to replicate the
  *ledger fragments that were stored there.
  * @param lId
  *Ledger id we want to recover.
- * @param ledgerIterCb
+ * @param dryrun
+ *printing the recovery plan without actually recovering 
bookies
+ * @param skipOpenLedgers
+ *Skip recovering open ledgers.
+ * @param finalLedgerIterCb
  *IterationCallback to invoke once we've recovered the current
  *ledger.
- * @param availableBookies
- *List of Bookie Servers that are available to use for
- *replicating data on the failed bookie. This could contain a
- *single bookie server if the user explicitly chose a bookie
- *server to replicate data to.
  */
-private void recoverLedger(final BookieSocketAddress bookieSrc, final long 
lId,
-final AsyncCallback.VoidCallback ledgerIterCb, final 
List availableBookies) {
+private void recoverLedger(final Set bookiesSrc, 
final long lId, final boolean dryrun,
+   final boolean skipOpenLedgers, final 
AsyncCallback.VoidCallback finalLedgerIterCb) {
 if (LOG.isDebugEnabled()) {
 LOG.debug("Recovering ledger : {}", lId);
 }
 
 asyncOpenLedgerNoRecovery(lId, new OpenCallback() {
 @Override
 public void openComplete(int rc, final LedgerHandle lh, Object 
ctx) {
-if (rc != Code.OK.intValue()) {
+if (rc != BKException.Code.OK) {
 LOG.error("BK error opening ledger: " + lId, 
BKException.create(rc));
-ledgerIterCb.processResult(rc, null, null);
+finalLedgerIterCb.processResult(rc, null, null);
 return;
 }
 
 LedgerMetadata lm = lh.getLedgerMetadata();
+if (skipOpenLedgers && !lm.isClosed() && !lm.isInRecovery()) {
+LOG.info("Skip recovering open ledger {}.", lId);
+try {
+lh.close();
+} catch (InterruptedException ie) {
+Thread.currentThread().interrupt();
+} catch (BKException bke) {
+LOG.warn("Error on closing ledger handle for {}.", 
lId);
+}
+finalLedgerIterCb.processResult(BKException.Code.OK, null, 
null);
+return;
+}
+
+boolean fenceRequired = false;
 
 Review comment:
   The change is to simplify the code and only update fenceRequired once. 
Rather than having containBookies take a List, change it to take a Map.Entry, 
which avoids all the lastKey stuff.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145548754
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -882,27 +941,74 @@ private void asyncRecoverLedgerFragment(final 
LedgerHandle lh,
  *- ledgerHandle
  * @param ledgerFragment
  *- LedgerFragment to replicate
- * @param targetBookieAddress
- *- target Bookie, to where entries should be replicated.
  */
 public void replicateLedgerFragment(LedgerHandle lh,
+final LedgerFragment ledgerFragment)
+throws InterruptedException, BKException {
+Optional excludedBookies = Optional.empty();
+Map targetBookieAddresses =
+getReplacedBookiesByIndexes(lh, ledgerFragment.getEnsemble(),
+ledgerFragment.getBookiesIndexes(), excludedBookies);
+replicateLedgerFragment(lh, ledgerFragment, targetBookieAddresses);
+}
+
+private void replicateLedgerFragment(LedgerHandle lh,
 final LedgerFragment ledgerFragment,
-final BookieSocketAddress targetBookieAddress)
+final Map targetBookieAddresses)
 
 Review comment:
   why are there multiple bookies being passed for a single fragment? A 
fragment is the subdivision of a segment that is stored on a single 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] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145502271
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -810,47 +780,71 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
  */
 for (final Long startEntryId : ledgerFragmentsToRecover) {
 Long endEntryId = ledgerFragmentsRange.get(startEntryId);
-BookieSocketAddress newBookie = null;
+ArrayList ensemble = 
lh.getLedgerMetadata().getEnsembles().get(startEntryId);
+// Get bookies to replace
+Map targetBookieAddresses;
 try {
-newBookie = 
getNewBookie(lh.getLedgerMetadata().getEnsembles().get(startEntryId),
- availableBookies);
-} catch (BKException.BKNotEnoughBookiesException bke) {
-
ledgerFragmentsMcb.processResult(BKException.Code.NotEnoughBookiesException,
- null, null);
+targetBookieAddresses = getReplacedBookies(lh, 
ensemble, bookiesSrc);
 
 Review comment:
   getReplacedBookies -> getReplacementBookies


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145542490
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -863,16 +857,81 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
  * @param ledgerFragmentMcb
  *- MultiCallback to invoke once we've recovered the current
  *ledger fragment.
- * @param newBookie
- *- New bookie we want to use to recover and replicate the
+ * @param newBookies
+ *- New bookies we want to use to recover and replicate the
  *ledger entries that were stored on the failed bookie.
  */
 private void asyncRecoverLedgerFragment(final LedgerHandle lh,
 final LedgerFragment ledgerFragment,
 final AsyncCallback.VoidCallback ledgerFragmentMcb,
-final BookieSocketAddress newBookie)
-throws InterruptedException {
-lfr.replicate(lh, ledgerFragment, ledgerFragmentMcb, newBookie);
+final Set newBookies) throws 
InterruptedException {
+lfr.replicate(lh, ledgerFragment, ledgerFragmentMcb, newBookies);
+}
+
+private Map getReplacedBookies(
+LedgerHandle lh,
+List ensemble,
+Set bookiesToRereplicate)
+throws BKException.BKNotEnoughBookiesException {
+Set bookieIndexesToRereplicate = Sets.newHashSet();
+for (int bookieIndex = 0; bookieIndex < ensemble.size(); 
bookieIndex++) {
+BookieSocketAddress bookieInEnsemble = ensemble.get(bookieIndex);
+if (bookiesToRereplicate.contains(bookieInEnsemble)) {
+bookieIndexesToRereplicate.add(bookieIndex);
+}
+}
+return getReplacedBookiesByIndexes(
+lh, ensemble, bookieIndexesToRereplicate, 
Optional.of(bookiesToRereplicate));
+}
+
+private Map getReplacedBookiesByIndexes(
 
 Review comment:
   likewise


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145501999
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -797,6 +763,10 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
 return;
 }
 
+if (dryrun) {
+System.out.println("Recovered ledger " + lId + " : " + 
(fenceRequired ? "[fence required]" : ""));
 
 Review comment:
   LOG


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145540992
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -810,47 +780,71 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
  */
 for (final Long startEntryId : ledgerFragmentsToRecover) {
 Long endEntryId = ledgerFragmentsRange.get(startEntryId);
-BookieSocketAddress newBookie = null;
+ArrayList ensemble = 
lh.getLedgerMetadata().getEnsembles().get(startEntryId);
+// Get bookies to replace
+Map targetBookieAddresses;
 try {
-newBookie = 
getNewBookie(lh.getLedgerMetadata().getEnsembles().get(startEntryId),
- availableBookies);
-} catch (BKException.BKNotEnoughBookiesException bke) {
-
ledgerFragmentsMcb.processResult(BKException.Code.NotEnoughBookiesException,
- null, null);
+targetBookieAddresses = getReplacedBookies(lh, 
ensemble, bookiesSrc);
+} catch (BKException.BKNotEnoughBookiesException e) {
+if (!dryrun) {
+
ledgerFragmentsMcb.processResult(BKException.Code.NotEnoughBookiesException, 
null, null);
+} else {
+System.out.println("  Fragment [" + startEntryId + 
" - " + endEntryId + " ] : "
+   + 
BKException.getMessage(BKException.Code.NotEnoughBookiesException));
+}
+continue;
+}
+
+if (dryrun) {
+ArrayList newEnsemble =
+replaceBookiesInEnsemble(ensemble, 
targetBookieAddresses);
+System.out.println("  Fragment [" + startEntryId + " - 
" + endEntryId + " ] : ");
+System.out.println("old ensemble : " + 
formatEnsemble(ensemble, bookiesSrc, '*'));
+System.out.println("new ensemble : " + 
formatEnsemble(newEnsemble, bookiesSrc, '*'));
 continue;
 }
 
 if (LOG.isDebugEnabled()) {
 LOG.debug("Replicating fragment from [" + startEntryId
   + "," + endEntryId + "] of ledger " + 
lh.getId()
-  + " to " + newBookie);
+  + " to " + targetBookieAddresses);
 }
 try {
 LedgerFragmentReplicator.SingleFragmentCallback cb = 
new LedgerFragmentReplicator.SingleFragmentCallback(
-   
ledgerFragmentsMcb, lh, startEntryId, bookieSrc, newBookie);
-ArrayList currentEnsemble = 
lh.getLedgerMetadata().getEnsemble(
-startEntryId);
-int bookieIndex = -1;
-if (null != currentEnsemble) {
-for (int i = 0; i < currentEnsemble.size(); i++) {
-if (currentEnsemble.get(i).equals(bookieSrc)) {
-bookieIndex = i;
-break;
-}
-}
-}
+ledgerFragmentsMcb, lh, startEntryId, 
getReplacedBookiesMap(ensemble, targetBookieAddresses));
 LedgerFragment ledgerFragment = new LedgerFragment(lh,
-startEntryId, endEntryId, bookieIndex);
-asyncRecoverLedgerFragment(lh, ledgerFragment, cb, 
newBookie);
+startEntryId, endEntryId, 
targetBookieAddresses.keySet());
+asyncRecoverLedgerFragment(lh, ledgerFragment, cb, 
Sets.newHashSet(targetBookieAddresses.values()));
 
 Review comment:
   It looks to me that you are recovering the segments even on a dryrun.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145500487
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -739,19 +684,40 @@ public void openComplete(int rc, final LedgerHandle lh, 
Object ctx) {
 asyncOpenLedger(lId, new OpenCallback() {
 @Override
 public void openComplete(int newrc, final 
LedgerHandle newlh, Object newctx) {
-if (newrc != Code.OK.intValue()) {
+if (newrc != BKException.Code.OK) {
 LOG.error("BK error close ledger: " + lId, 
BKException.create(newrc));
-ledgerIterCb.processResult(newrc, null, 
null);
+finalLedgerIterCb.processResult(newrc, 
null, null);
 return;
 }
-// do recovery
-recoverLedger(bookieSrc, lId, ledgerIterCb, 
availableBookies);
+bkc.mainWorkerPool.submit(() -> {
+// do recovery
+recoverLedger(bookiesSrc, lId, dryrun, 
skipOpenLedgers, finalLedgerIterCb);
+});
 }
 }, null);
 return;
 }
 }
 
+final AsyncCallback.VoidCallback ledgerIterCb = new 
AsyncCallback.VoidCallback() {
 
 Review comment:
   Move this down to where it's first used. Also, 
   final AsyncCallback.VoidCallback ledgerIterCb = (rc, path, ctx) -> {...}


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145551983
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationStats.java
 ##
 @@ -43,6 +43,7 @@
 public final static String NUM_BYTES_READ = "NUM_BYTES_READ";
 public final static String NUM_ENTRIES_WRITTEN = "NUM_ENTRIES_WRITTEN";
 public final static String NUM_BYTES_WRITTEN = "NUM_BYTES_WRITTEN";
+   public final static String REPLICATE_EXCEPTION = "exceptions";
 
 Review comment:
   tab


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145502370
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -810,47 +780,71 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
  */
 for (final Long startEntryId : ledgerFragmentsToRecover) {
 Long endEntryId = ledgerFragmentsRange.get(startEntryId);
-BookieSocketAddress newBookie = null;
+ArrayList ensemble = 
lh.getLedgerMetadata().getEnsembles().get(startEntryId);
+// Get bookies to replace
+Map targetBookieAddresses;
 try {
-newBookie = 
getNewBookie(lh.getLedgerMetadata().getEnsembles().get(startEntryId),
- availableBookies);
-} catch (BKException.BKNotEnoughBookiesException bke) {
-
ledgerFragmentsMcb.processResult(BKException.Code.NotEnoughBookiesException,
- null, null);
+targetBookieAddresses = getReplacedBookies(lh, 
ensemble, bookiesSrc);
+} catch (BKException.BKNotEnoughBookiesException e) {
+if (!dryrun) {
+
ledgerFragmentsMcb.processResult(BKException.Code.NotEnoughBookiesException, 
null, null);
+} else {
+System.out.println("  Fragment [" + startEntryId + 
" - " + endEntryId + " ] : "
 
 Review comment:
   LOG
   


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145542430
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -863,16 +857,81 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
  * @param ledgerFragmentMcb
  *- MultiCallback to invoke once we've recovered the current
  *ledger fragment.
- * @param newBookie
- *- New bookie we want to use to recover and replicate the
+ * @param newBookies
+ *- New bookies we want to use to recover and replicate the
  *ledger entries that were stored on the failed bookie.
  */
 private void asyncRecoverLedgerFragment(final LedgerHandle lh,
 final LedgerFragment ledgerFragment,
 final AsyncCallback.VoidCallback ledgerFragmentMcb,
-final BookieSocketAddress newBookie)
-throws InterruptedException {
-lfr.replicate(lh, ledgerFragment, ledgerFragmentMcb, newBookie);
+final Set newBookies) throws 
InterruptedException {
+lfr.replicate(lh, ledgerFragment, ledgerFragmentMcb, newBookies);
+}
+
+private Map getReplacedBookies(
 
 Review comment:
   Ah, this actually mutates the ensemble. I would call this 
replaceBookiesAndGet, so that it is clear that mutation is happening.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145499216
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -672,55 +619,52 @@ public void process(Long ledgerId, 
AsyncCallback.VoidCallback iterCallback) {
 }
 
 /**
- * Get a new random bookie, but ensure that it isn't one that is already
- * in the ensemble for the ledger.
- */
-private BookieSocketAddress getNewBookie(final List 
bookiesAlreadyInEnsemble,
-final List availableBookies)
-throws BKException.BKNotEnoughBookiesException {
-ArrayList candidates = new 
ArrayList();
-candidates.addAll(availableBookies);
-candidates.removeAll(bookiesAlreadyInEnsemble);
-if (candidates.size() == 0) {
-throw new BKException.BKNotEnoughBookiesException();
-}
-return candidates.get(rand.nextInt(candidates.size()));
-}
-
-/**
  * This method asynchronously recovers a given ledger if any of the ledger
  * entries were stored on the failed bookie.
  *
- * @param bookieSrc
- *Source bookie that had a failure. We want to replicate the
+ * @param bookiesSrc
+ *Source bookies that had a failure. We want to replicate the
  *ledger fragments that were stored there.
  * @param lId
  *Ledger id we want to recover.
- * @param ledgerIterCb
+ * @param dryrun
+ *printing the recovery plan without actually recovering 
bookies
+ * @param skipOpenLedgers
+ *Skip recovering open ledgers.
+ * @param finalLedgerIterCb
  *IterationCallback to invoke once we've recovered the current
  *ledger.
- * @param availableBookies
- *List of Bookie Servers that are available to use for
- *replicating data on the failed bookie. This could contain a
- *single bookie server if the user explicitly chose a bookie
- *server to replicate data to.
  */
-private void recoverLedger(final BookieSocketAddress bookieSrc, final long 
lId,
-final AsyncCallback.VoidCallback ledgerIterCb, final 
List availableBookies) {
+private void recoverLedger(final Set bookiesSrc, 
final long lId, final boolean dryrun,
+   final boolean skipOpenLedgers, final 
AsyncCallback.VoidCallback finalLedgerIterCb) {
 if (LOG.isDebugEnabled()) {
 LOG.debug("Recovering ledger : {}", lId);
 }
 
 asyncOpenLedgerNoRecovery(lId, new OpenCallback() {
 @Override
 public void openComplete(int rc, final LedgerHandle lh, Object 
ctx) {
-if (rc != Code.OK.intValue()) {
+if (rc != BKException.Code.OK) {
 LOG.error("BK error opening ledger: " + lId, 
BKException.create(rc));
-ledgerIterCb.processResult(rc, null, null);
+finalLedgerIterCb.processResult(rc, null, null);
 return;
 }
 
 LedgerMetadata lm = lh.getLedgerMetadata();
+if (skipOpenLedgers && !lm.isClosed() && !lm.isInRecovery()) {
+LOG.info("Skip recovering open ledger {}.", lId);
+try {
+lh.close();
+} catch (InterruptedException ie) {
+Thread.currentThread().interrupt();
+} catch (BKException bke) {
+LOG.warn("Error on closing ledger handle for {}.", 
lId);
+}
+finalLedgerIterCb.processResult(BKException.Code.OK, null, 
null);
+return;
+}
+
+boolean fenceRequired = false;
 
 Review comment:
   final boolean fenceRequired = containBookies(lm.getEnsembles().lastEntry(), 
bookieSrc);
   
   and make containsBookie handle null. 


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145499417
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -739,19 +684,40 @@ public void openComplete(int rc, final LedgerHandle lh, 
Object ctx) {
 asyncOpenLedger(lId, new OpenCallback() {
 
 Review comment:
   asyncOpenLedger(lId, (newrc, newlh, newctx) -> {...})


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145501811
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -739,19 +684,40 @@ public void openComplete(int rc, final LedgerHandle lh, 
Object ctx) {
 asyncOpenLedger(lId, new OpenCallback() {
 @Override
 public void openComplete(int newrc, final 
LedgerHandle newlh, Object newctx) {
-if (newrc != Code.OK.intValue()) {
+if (newrc != BKException.Code.OK) {
 LOG.error("BK error close ledger: " + lId, 
BKException.create(newrc));
-ledgerIterCb.processResult(newrc, null, 
null);
+finalLedgerIterCb.processResult(newrc, 
null, null);
 return;
 }
-// do recovery
-recoverLedger(bookieSrc, lId, ledgerIterCb, 
availableBookies);
+bkc.mainWorkerPool.submit(() -> {
+// do recovery
+recoverLedger(bookiesSrc, lId, dryrun, 
skipOpenLedgers, finalLedgerIterCb);
+});
 }
 }, null);
 return;
 }
 }
 
+final AsyncCallback.VoidCallback ledgerIterCb = new 
AsyncCallback.VoidCallback() {
 
 Review comment:
   Actually, i'd move the definition out to a utility method to clarify what 
the wrapper is for.
   VoidCallback closeOnCompleteCb(LedgerHandle lh, VoidCallback cb);


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145549021
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -882,27 +941,74 @@ private void asyncRecoverLedgerFragment(final 
LedgerHandle lh,
  *- ledgerHandle
  * @param ledgerFragment
  *- LedgerFragment to replicate
- * @param targetBookieAddress
- *- target Bookie, to where entries should be replicated.
  */
 public void replicateLedgerFragment(LedgerHandle lh,
+final LedgerFragment ledgerFragment)
+throws InterruptedException, BKException {
+Optional excludedBookies = Optional.empty();
+Map targetBookieAddresses =
+getReplacedBookiesByIndexes(lh, ledgerFragment.getEnsemble(),
+ledgerFragment.getBookiesIndexes(), excludedBookies);
+replicateLedgerFragment(lh, ledgerFragment, targetBookieAddresses);
+}
+
+private void replicateLedgerFragment(LedgerHandle lh,
 final LedgerFragment ledgerFragment,
-final BookieSocketAddress targetBookieAddress)
+final Map targetBookieAddresses)
 
 Review comment:
   Ah, you're changing the definition of fragment later on too.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145495100
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -663,7 +610,7 @@ public void processResult(int rc, String path, Object ctx) 
{
 Processor ledgerProcessor = new Processor() {
 
 Review comment:
   Likewise, we don't need to create an instance, just use a lambda
   (lid, cb) -> { recoverLedger(bookiesSrc, ledgerId, dryrun, skipOpenLedgers, 
iterCallback); }


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145494666
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -626,25 +577,21 @@ public void processResult(int rc, String path, Object 
ctx, List children
  * determine if any of the ledger fragments for it were stored at the dead
  * input bookie.
  *
- * @param bookieSrc
- *Source bookie that had a failure. We want to replicate the
+ * @param bookiesSrc
+ *Source bookies that had a failure. We want to replicate the
  *ledger fragments that were stored there.
- * @param bookieDest
- *Optional destination bookie that if passed, we will copy all
- *of the ledger fragments from the source bookie over to it.
+ * @param dryrun
+ *dryrun the recover procedure.
+ * @param skipOpenLedgers
+ *Skip recovering open ledgers.
  * @param cb
  *RecoverCallback to invoke once all of the data on the dead
  *bookie has been recovered and replicated.
  * @param context
  *Context for the RecoverCallback to call.
- * @param availableBookies
- *List of Bookie Servers that are available to use for
- *replicating data on the failed bookie. This could contain a
- *single bookie server if the user explicitly chose a bookie
- *server to replicate data to.
  */
-private void getActiveLedgers(final BookieSocketAddress bookieSrc, final 
BookieSocketAddress bookieDest,
-final RecoverCallback cb, final Object context, final 
List availableBookies) {
+private void getActiveLedgers(final Set bookiesSrc, 
final boolean dryrun,
+  final boolean skipOpenLedgers, final 
RecoverCallback cb, final Object context) {
 // Wrapper class around the RecoverCallback so it can be used
 // as the final VoidCallback to process ledgers
 class RecoverCallbackWrapper implements AsyncCallback.VoidCallback {
 
 Review comment:
   Doesn't need to happen as part of this PR, but this could be replaced by a 
lambda.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145492698
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -495,9 +498,20 @@ public SyncObject() {
  */
 public void recoverBookieData(final BookieSocketAddress bookieSrc, final 
BookieSocketAddress bookieDest)
 
 Review comment:
   We should deprecate this call, since it's no different to recoverBookieData.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145493594
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -495,9 +498,20 @@ public SyncObject() {
  */
 public void recoverBookieData(final BookieSocketAddress bookieSrc, final 
BookieSocketAddress bookieDest)
 
 Review comment:
   or remove it completely, since the patch does remove the async version, 
though this would be a BC break.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145493659
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -535,89 +549,26 @@ public void recoverComplete(int rc, Object ctx) {
  * @param bookieSrc
  *Source bookie that had a failure. We want to replicate the
  *ledger fragments that were stored there.
- * @param bookieDest
- *Optional destination bookie that if passed, we will copy all
- *of the ledger fragments from the source bookie over to it.
  * @param cb
  *RecoverCallback to invoke once all of the data on the dead
  *bookie has been recovered and replicated.
  * @param context
  *Context for the RecoverCallback to call.
  */
-public void asyncRecoverBookieData(final BookieSocketAddress bookieSrc, 
final BookieSocketAddress bookieDest,
+public void asyncRecoverBookieData(final BookieSocketAddress bookieSrc,
 
 Review comment:
   BC break.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145543784
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
 ##
 @@ -863,16 +857,81 @@ public void openComplete(int newrc, final LedgerHandle 
newlh, Object newctx) {
  * @param ledgerFragmentMcb
  *- MultiCallback to invoke once we've recovered the current
  *ledger fragment.
- * @param newBookie
- *- New bookie we want to use to recover and replicate the
+ * @param newBookies
+ *- New bookies we want to use to recover and replicate the
  *ledger entries that were stored on the failed bookie.
  */
 private void asyncRecoverLedgerFragment(final LedgerHandle lh,
 final LedgerFragment ledgerFragment,
 final AsyncCallback.VoidCallback ledgerFragmentMcb,
-final BookieSocketAddress newBookie)
-throws InterruptedException {
-lfr.replicate(lh, ledgerFragment, ledgerFragmentMcb, newBookie);
+final Set newBookies) throws 
InterruptedException {
+lfr.replicate(lh, ledgerFragment, ledgerFragmentMcb, newBookies);
+}
+
+private Map getReplacedBookies(
 
 Review comment:
   Longterm, I think we should stop mutating the ensembles like this. The 
ensemble for a segment is the ensemble of bookies that "vote" on an entry. It 
should be immutable. When we rereplicate, we should add another field to the 
metadata for "extraReplicas", and we can then merge on the read. This would 
simplify a lot of the recovery logic around ensemble changes.


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


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-18 Thread GitBox 
ivankelly commented on a change in pull request #641: Issue-596 Issue-583: Auto 
replication should honor ensemble placement policy
URL: https://github.com/apache/bookkeeper/pull/641#discussion_r145551162
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java
 ##
 @@ -94,13 +95,13 @@ private void replicateFragmentInternal(final LedgerHandle 
lh,
  * Ideally this should never happen if bookie failure is taken care
  * of properly. Nothing we can do though in this case.
  */
-LOG.warn("Dead bookie (" + lf.getAddress()
+LOG.warn("Dead bookie (" + lf.getAddresses()
 + ") is still part of the current"
 + " active ensemble for ledgerId: " + lh.getId());
 ledgerFragmentMcb.processResult(BKException.Code.OK, null, null);
 return;
 }
-if (startEntryId > endEntryId) {
+if (startEntryId > endEntryId || endEntryId <= -1L) {
 
 Review comment:
   use INVALID_ENTRY_ID rather than -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