[GitHub] sijie commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233207589
##
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:
No, what I mean is underlying implementation will probably already handle
that. we don't need to worry about that at this layer.
it is a good feature to have at any case. but let's do it in a separate PR
rather than doing it in a large refactor.
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 #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233207255
##
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:
Yeah. I would prefer creating a future each time, that would be a clean
path, and will be no surprise from gc perspective.
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 #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233128066
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
##
@@ -238,34 +239,31 @@ public void run() {
if (LOG.isDebugEnabled()) {
LOG.debug("Re-read ledger metadata for {}.", ledgerId);
}
-readLedgerMetadata(ledgerId, ReadLedgerMetadataTask.this);
+readLedgerMetadata(ledgerId).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 metadata,
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.getValue());
+LOG.debug("Ledger metadata is changed for {} :
{}.", ledgerId, metadata.getValue());
}
-scheduler.submit(new Runnable() {
-@Override
-public void run() {
+scheduler.submit(() -> {
synchronized (listenerSet) {
for (LedgerMetadataListener listener :
listenerSet) {
-listener.onChanged(ledgerId, result);
+listener.onChanged(ledgerId, metadata);
}
}
-}
-});
+});
}
-} else if (BKException.Code.NoSuchLedgerExistsException == rc)
{
+} else if (exception instanceof
BKException.BKNoSuchLedgerExistsException) {
Review comment:
use `getExceptionCode` to check exception code.
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 #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233121099
##
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:
isn't exception here a `CompletionException`?
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 #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233123746
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java
##
@@ -144,7 +143,7 @@ public void run() {
}
}
} else {
-LOG.warn("Failed on read ledger metadata of ledger {} : {}",
ledgerId, rc);
+LOG.warn("Failed on read ledger metadata of ledger {}",
ledgerId, exception);
Review comment:
I don't think the exception stack trace is useful here (in async callbacks).
so I think we should still use `rc` here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233128211
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
##
@@ -275,7 +273,7 @@ public void run() {
}
}
} else {
-LOG.warn("Failed on read ledger metadata of ledger {} :
{}", ledgerId, rc);
+LOG.warn("Failed on read ledger metadata of ledger {}",
ledgerId, exception);
Review comment:
the exception stack trace is not useful.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233123083
##
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:
I will suggest not using `instanceof` here. We should just use
`getExceptionCode(exception) == BKException.Code.NoSuchLedgerExistsException`
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 #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233124668
##
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:
don't do this. you can have a static method creating the future, but never
keep it as a static variable.
every future should be a brand new instance, otherwise, all the callbacks
will be registered here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233120404
##
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:
can you explain why we are adding this? and please add a comment here
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233118965
##
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:
I think the exception would be `CompletionException` here. So I am not sure
how `if (exception instanceof BKException.BKNoSuchLedgerExistsException)` will
be triggered. (unless I misunderstand `whenComplete`)
see:
https://github.com/apache/bookkeeper/blob/master/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/concurrent/FutureEventListener.java#L35
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 #1809: Change LedgerManager to use CompletableFuture
sijie commented on a change in pull request #1809: Change LedgerManager to use
CompletableFuture
URL: https://github.com/apache/bookkeeper/pull/1809#discussion_r233127634
##
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:
I am not sure we need this or not. The underlying LM should error about its
complete futures, so all the complete futures at this class will be errored out
correctly. The change here is masking any potentially errors in underlying LM.
I would suggest not adding such logic for now. or at least don't do it in a
refactor PR.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
