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

2017-10-25 Thread GitBox 
sijie 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_r146773883
 
 

 ##
 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 getReplacementBookies(
+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 getReplacementBookiesByIndexes(
+lh, ensemble, bookieIndexesToRereplicate, 
Optional.of(bookiesToRereplicate));
+}
+
+private Map getReplacementBookiesByIndexes(
+LedgerHandle lh,
+List ensemble,
+Set bookieIndexesToRereplicate,
+Optional excludedBookies)
+throws BKException.BKNotEnoughBookiesException {
+// target bookies to replicate
+Map targetBookieAddresses =
+
Maps.newHashMapWithExpectedSize(bookieIndexesToRereplicate.size());
+// bookies to exclude for ensemble allocation
+Set bookiesToExclude = Sets.newHashSet();
+if (excludedBookies.isPresent()) {
+bookiesToExclude.addAll(excludedBookies.get());
+}
+
+// excluding bookies that need to be replicated
+for (Integer bookieIndex : bookieIndexesToRereplicate) {
+BookieSocketAddress bookie = ensemble.get(bookieIndex);
+bookiesToExclude.add(bookie);
+}
+
+// allocate bookies
+for (Integer bookieIndex : bookieIndexesToRereplicate) {
+BookieSocketAddress oldBookie = ensemble.get(bookieIndex);
+BookieSocketAddress newBookie =
+bkc.getPlacementPolicy().replaceBookie(
 
 Review comment:
   yes and no. it is sort of cluster-wide, but it is actually per client. if 
your clients share same configuration, then it is same placement policy.
   
   This placement policy has to be set in the configuration for auditors and 
replication workers. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-25 Thread GitBox 
sijie 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_r146773358
 
 

 ##
 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:
   @jvrao we were only talking about the output introduced by `dryrun`. we were 
not discussing changing the existing logging behavior.


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


With regards,
Apache Git Services

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

2017-10-24 Thread GitBox 
sijie 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_r146758705
 
 

 ##
 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:
   `dryrun` is a specified by the caller, it is a contract between the caller 
and the implementation. if someone is using BookKeeperAdmin in some service, 
they are aware of this behavior since it is a contract provided by the admin. 
the behavior is expected and determined when using `stdout`.
   
   but using log4j for tool command results make the behavior is a bit 
undefined. because the whole behavior is tied with a log4j.properties file. it 
is fine if people using the bk shell, but behavior would become undefined if 
people integrates BookKeeperAdmin in their tools. they have to be aware of the 
implementation of `dryrun` and customize their log4j properties accordingly.
   
   anyway, I will change this to log4j to move forward here and update bkshell 
accordingly in the subsequent change.
   


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


With regards,
Apache Git Services

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

2017-10-24 Thread GitBox 
sijie 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_r146480717
 
 

 ##
 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:
   still isn't clear to me. fenceRequired is only set when 1) a ledger is not 
closed 2) last ensemble contain the src bookie. are you suggesting me pushing 
both 1) and 2) into containBookies function? this doesn't sound correct to me 
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 a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
sijie 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_r146479924
 
 

 ##
 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:
   okay. I would leave it know and have #655 clean this up later. is that okay?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-24 Thread GitBox 
sijie 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_r146479659
 
 

 ##
 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:
   @ivankelly it uses log4j. the dryrun is printing on the console rather than 
using log4j, so the information will not be mixed with other log4j logging. the 
`dryrun` is kind of printing an execution plan. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146360130
 
 

 ##
 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:
   actually I keep this method unchanged is because BookieShell is using this 
method. I am changing bookie shell in the subsequent change, so I leave this 
method unchanged.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146359096
 
 

 ##
 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:
   I don't think it is worth a util method, because the logging info is more 
recover related. we can improve it using lambda and completable future in 
future.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146357086
 
 

 ##
 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:
   not sure I understand this comment. I don't see the difference of your 
suggestion and my change. there isn't null validation here 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 a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146355670
 
 

 ##
 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) {
 
 Review comment:
   because `dryrun` is to dump all the information, which it doesn't mutate any 
state. it doesn't hurt to continue the `dryrun` when hitting not enough bookies 
exception. especially, not enough bookies exception can be thrown when it 
doesn't meet rack/region placement constraints.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146355111
 
 

 ##
 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:
   no. for each ledger, `dryrun` stops at printing messages at line 804.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146354598
 
 

 ##
 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:
   hmm, where do you see the mutation happening? this method is calculating the 
bookies to replace based on ensemble. the actual mutation of ledger metadata 
only happens after successful re-replication.
   
   `extraReplicas` would actually introduce complexity on reading, garbage 
collection and etc. it is actually not too bad for the ledgers based on a 
metadata store. it can be bad for `log0` stuff, but the ledgers in `log0` can 
be a special case that disable ensemble change which a ledger is effectively an 
ensemble.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146352189
 
 

 ##
 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:
   yes, a fragment is a range of ledger entries. it is not per bookie. so when 
we replicate a fragment, it can be more efficient on replicating to multiple 
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] sijie commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146351206
 
 

 ##
 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:
   this was made to use `System.out` for dryrun in bk shell. I have a upcoming 
change in bk shell to use `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] sijie commented on a change in pull request #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146350662
 
 

 ##
 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:
   https://github.com/apache/bookkeeper/issues/655


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146350639
 
 

 ##
 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:
   created: https://github.com/apache/bookkeeper/issues/655


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #641: Issue-596 Issue-583: Auto replication should honor ensemble placement policy

2017-10-23 Thread GitBox 
sijie 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_r146349344
 
 

 ##
 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:
   I will remove this method. we don't define the binary backward compatibility 
for bookkeeper admin, and I doubt people depends on bookkeeper admin 
programmatically. 


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


With regards,
Apache Git Services