[GitHub] ivankelly commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r234236380
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java
##
@@ -439,12 +439,22 @@ public BKLedgerIdOverflowException() {
* Extract an exception code from an BKException, or use a default if it's
another type.
*/
public static int getExceptionCode(Throwable t, int defaultCode) {
-if (t instanceof BKException) {
+if (t == null) {
+return BKException.Code.OK;
+} else if (t instanceof BKException) {
return ((BKException) t).getCode();
} else if (t.getCause() != null) {
Review comment:
You'll get CompletionException wrapping, if the future you put your handler
is not the future on which completeExceptionally is called.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r234229005
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
##
@@ -491,26 +489,24 @@ public void asyncGetLedgersContainBookies(final
Set bookies
bkc.getLedgerManager().asyncProcessLedgers(new Processor() {
@Override
public void process(final Long lid, final
AsyncCallback.VoidCallback cb) {
-bkc.getLedgerManager().readLedgerMetadata(lid, new
GenericCallback>() {
-@Override
-public void operationComplete(int rc,
Versioned metadata) {
-if (BKException.Code.NoSuchLedgerExistsException ==
rc) {
-// the ledger was deleted during this iteration.
-cb.processResult(BKException.Code.OK, null, null);
-return;
-} else if (BKException.Code.OK != rc) {
-cb.processResult(rc, null, null);
-return;
-}
-Set bookiesInLedger =
metadata.getValue().getBookiesInThisLedger();
-Sets.SetView intersection =
+bkc.getLedgerManager().readLedgerMetadata(lid)
+.whenComplete((metadata, exception) -> {
+if (exception instanceof
BKException.BKNoSuchLedgerExistsException) {
Review comment:
No, shouldn't be, but it's dependent on how readLedgerMetadata is
implemented, so will change to getCode
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r234228770
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java
##
@@ -439,12 +439,22 @@ public BKLedgerIdOverflowException() {
* Extract an exception code from an BKException, or use a default if it's
another type.
*/
public static int getExceptionCode(Throwable t, int defaultCode) {
-if (t instanceof BKException) {
+if (t == null) {
Review comment:
Will add to javadoc
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r234228416
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
##
@@ -1024,41 +1022,45 @@ public int runCmd(CommandLine cmdLine) throws
Exception {
final CountDownLatch processDone = new CountDownLatch(1);
Processor ledgerProcessor = new Processor() {
-@Override
-public void process(Long ledgerId, VoidCallback cb) {
-if (!printMeta && (bookieAddress == null)) {
-printLedgerMetadata(ledgerId, null, false);
-cb.processResult(BKException.Code.OK, null,
null);
-} else {
-GenericCallback>
gencb = (rc, ledgerMetadata) -> {
-if (rc == BKException.Code.OK) {
-if ((bookieAddress == null)
-||
BookKeeperAdmin.areEntriesOfLedgerStoredInTheBookie(
-ledgerId, bookieAddress,
ledgerMetadata.getValue())) {
-/*
- * the print method has to be in
- * synchronized scope, otherwise
- * output of printLedgerMetadata
- * could interleave since this
- * callback for different
- * ledgers can happen in
- * different threads.
- */
-synchronized (BookieShell.this) {
-printLedgerMetadata(ledgerId,
ledgerMetadata.getValue(), printMeta);
-}
-}
-} else if (rc ==
BKException.Code.NoSuchLedgerExistsException) {
-rc = BKException.Code.OK;
-} else {
-LOG.error("Unable to read the ledger:
" + ledgerId + " information");
-}
-cb.processResult(rc, null, null);
-};
-ledgerManager.readLedgerMetadata(ledgerId,
gencb);
+@Override
+public void process(Long ledgerId, VoidCallback
cb) {
+if (!printMeta && (bookieAddress == null)) {
+printLedgerMetadata(ledgerId, null, false);
+cb.processResult(BKException.Code.OK,
null, null);
+} else {
+
ledgerManager.readLedgerMetadata(ledgerId).whenComplete(
+(metadata, exception) -> {
Review comment:
in this case, it would be a BKException, though when exceptions are wrapped
and when they are not is not straightforward.
If I have an future X and I call completeExceptionally on it, all handlers
for that exception will get the raw exception. Any futures which have been
created when installing the handlers will get the wrapped exception.
In any case, I'm going remove all the instanceof and just pull the error
codes out
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r234209071
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java
##
@@ -108,64 +115,69 @@ private GenericCallback removeCallback(GenericCallback
callback) {
return callbacks.remove(callback);
}
+private void recordPromise(CompletableFuture promise) {
Review comment:
the underlying implementations don't handle it though. for them to handle
it, each would have to have this recording logic. so it's cleaner to have it
abstracted somewhere else.
The reason I did it in this patch is so this patch doesn't remove
functionality. Currently, the cleanup lm ensures that if the lm is closed, then
all outstanding requests are completed. If we don't implement this here, then
this functionality is lost.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233205362
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java
##
@@ -108,64 +115,69 @@ private GenericCallback removeCallback(GenericCallback
callback) {
return callbacks.remove(callback);
}
+private void recordPromise(CompletableFuture promise) {
Review comment:
Are you saying the underlying LM should keep track of all futures they
return?
This change is to basically do the same thing as the CleanupLedgerManager
did for callbacks. i.e. If close is called, all outstanding futures should be
closed, and all future calls also closed. Moving this into the underlying LM
would basically mean making an AbstractLM with this recordPromise and pulling
that into the other LMs.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to 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 #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233204066
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java
##
@@ -75,11 +76,17 @@ public LedgerRange next() throws IOException {
}
}
+private static final CompletableFuture>
closedPromise = new CompletableFuture<>();
Review comment:
https://github.com/bpupadhyaya/openjdk-8/blob/45af329463a45955ea2759b89cb0ebfe40570c3f/jdk/src/share/classes/java/util/concurrent/CompletableFuture.java#L2015
If the future is complete, nothing is queued. It's executed straight away.
I've no problem creating a future each time, this isn't fast path code, but it
is safe to use a future like this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
ivankelly commented on a change in pull request #1809: Change LedgerManager to
use CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233203737
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
##
@@ -100,34 +102,31 @@ public void run() {
if (LOG.isDebugEnabled()) {
LOG.debug("Re-read ledger metadata for {}.", ledgerId);
}
-readLedgerMetadata(ledgerId, this,
AbstractZkLedgerManager.this);
+readLedgerMetadata(ledgerId, AbstractZkLedgerManager.this)
+.whenComplete((metadata, exception) ->
handleMetadata(metadata, exception));
} else {
if (LOG.isDebugEnabled()) {
LOG.debug("Ledger metadata listener for ledger {} is
already removed.", ledgerId);
}
}
}
-@Override
-public void operationComplete(int rc, final Versioned
result) {
-if (BKException.Code.OK == rc) {
+private void handleMetadata(Versioned result,
Throwable exception) {
+if (exception == null) {
final Set listenerSet =
listeners.get(ledgerId);
if (null != listenerSet) {
if (LOG.isDebugEnabled()) {
LOG.debug("Ledger metadata is changed for {} : {}.",
ledgerId, result);
}
-scheduler.submit(new Runnable() {
-@Override
-public void run() {
+scheduler.submit(() -> {
synchronized (listenerSet) {
for (LedgerMetadataListener listener :
listenerSet) {
listener.onChanged(ledgerId, result);
}
}
-}
-});
+});
}
-} else if (BKException.Code.NoSuchLedgerExistsException == rc) {
+} else if (exception instanceof
BKException.BKNoSuchLedgerExistsException) {
Review comment:
Ya, I'm going to change any place I have instanceof to getExceptionCode. The
CompletionException stuff is too unclear.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
