You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/12/01 13:11:58 UTC
[GitHub] [bookkeeper] wenbingshen opened a new pull request, #3683: Not wrap IOException twice form checkpoint
wenbingshen opened a new pull request, #3683:
URL: https://github.com/apache/bookkeeper/pull/3683
### Motivation
IOException may be wrapped by RuntimeException, so don't wrap RuntimeException with IOException.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#issuecomment-1378324646
@wenbingshen Would you please resolve the conflicts? thanks a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1041671081
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1037946674
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
IOException is already caught at the first catch block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 merged pull request #3683: Not wrap IOException twice form checkpoint
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 merged PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] wenbingshen commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1045471056
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
@horizonzy @hangc0276 addressed done. PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1037946674
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
Does IOException throw at the first catch block?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1039082376
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
Could you share the detailed IOException wrapped by RuntimeException?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] wenbingshen commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1039102570
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
@hangc0276 As long as I understand correctly, I believe ` writeCacheBeingFlushed forEach ` throws an `IOException` wrapped with a `RuntimeException`:
```java https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L782-L789
writeCacheBeingFlushed.forEach((ledgerId, entryId, entry) -> {
try {
long location = entryLogger.addEntry(ledgerId, entry);
entryLocationIndex.addLocation(batch, ledgerId, entryId, location);
} catch (IOException e) {
throw new RuntimeException(e);
}
});
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
@hangc0276 As long as I understand correctly, I believe ` writeCacheBeingFlushed forEach ` throws an `IOException` wrapped with a `RuntimeException`:
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L782-L789
```java
writeCacheBeingFlushed.forEach((ledgerId, entryId, entry) -> {
try {
long location = entryLogger.addEntry(ledgerId, entry);
entryLocationIndex.addLocation(batch, ledgerId, entryId, location);
} catch (IOException e) {
throw new RuntimeException(e);
}
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1039125468
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
Change the interface will be better:
```
public interface EntryConsumer {
void accept(long ledgerId, long entryId, ByteBuf entry);
}
```
```
public interface EntryConsumer {
void accept(long ledgerId, long entryId, ByteBuf entry) throws IOException;
}
```
So we needn't catch the inner exception. Throw the IOException directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] wenbingshen commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1039102570
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
@hangc0276 As long as I understand correctly, I believe ` writeCacheBeingFlushed forEach ` throws an `IOException` wrapped with a `RuntimeException`:
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L782-L789
```java
writeCacheBeingFlushed.forEach((ledgerId, entryId, entry) -> {
try {
long location = entryLogger.addEntry(ledgerId, entry);
entryLocationIndex.addLocation(batch, ledgerId, entryId, location);
} catch (IOException e) {
throw new RuntimeException(e);
}
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] wenbingshen commented on a diff in pull request #3683: Not wrap IOException twice form checkpoint
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3683:
URL: https://github.com/apache/bookkeeper/pull/3683#discussion_r1038018258
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -839,6 +839,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
throw e;
} catch (RuntimeException e) {
recordFailedEvent(dbLedgerStorageStats.getFlushStats(), startTime);
+ // not wrap IOException again
+ if (e.getCause() != null && e.getCause() instanceof IOException) {
Review Comment:
This IOException is wrapped by RuntimeException. not a directly IOException. The first catch block can not caught it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org