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/10/20 13:14:35 UTC
[GitHub] [bookkeeper] poorbarcode opened a new pull request, #3562: When call openLedgerOp, make the timeout ex is a separate error code
poorbarcode opened a new pull request, #3562:
URL: https://github.com/apache/bookkeeper/pull/3562
Descriptions of the changes in this PR:
### Motivation
When we execute `bkClient.openLedger(ledgerId)`, the execution flow is as follows：
1. start opening the ledger
2. get ledger meta
3. read the last confirmed entry
4. open ledger success
If we get the correct ledgerMeta at step 2, this means that this ledger has not been deleted. If step 3 times out, we should try again to make sure the ledger exists until we get a clear response from the BK server.<strong>(High light)</strong>However, in the current implementation, the timeout exception is rewritten as a `LedgerRecoveryException`, making it impossible to determine whether we should retry.
Log:
```
Oct 17, 2022 22:54:05.818 [BookKeeperClientWorker-OrderedExecutor-16-0] ERROR org.apache.bookkeeper.client.ReadLastConfirmedOp - While readLastConfirmed ledger: 59158316 did not hear success responses from all quorums, QuorumCoverage(e:2,w:2,a:2) = [-23, -23]
Oct 17, 2022 22:54:05.818 [BookKeeperClientWorker-OrderedExecutor-16-0] INFO org.apache.bookkeeper.mledger.impl.ManagedCursorImpl - [order/org-217/persistent/p1010-tx] Opened ledger 59158316 for consumer order-service. rc=-10
```
Looking at the ledger metadata:
```
LedgerMetadata{formatVersion=3, ensembleSize=2, writeQuorumSize=2, ackQuorumSize=2, state=IN_RECOVERY, digestType=CRC32C, password=base64:, ensembles={0=[***:3181, ***:3181]}, customMetadata={component=***, pulsar/managed-ledger=***, pulsar/cursor=***, application=***}}
```
### Changes
When calling openLedgerOp, do not rewritten `TiemoutException` as a `LedgerRecoveryException`
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1006370475
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -331,47 +330,6 @@ public void testOpenLedgerClientClosed() throws Exception {
.execute());
}
- @Test
Review Comment:
These two tests has been removed into `BookKeeperBuildersOpenLedgerTest.java`. see https://github.com/apache/bookkeeper/pull/3562#discussion_r1005554159
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1001338409
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java:
##########
@@ -207,10 +207,19 @@ public void safeOperationComplete(int rc, Void result) {
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
- openComplete(BKException.Code.UnauthorizedAccessException, null);
+ openComplete(rc, null);
+ });
+ } else if (rc == BKException.Code.TimeoutException) {
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ if (ex != null) {
+ LOG.error("Ledger {} read timeout", ledgerId, ex);
+ }
+ openComplete(rc, null);
});
Review Comment:
You are right, already fixed
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java:
##########
@@ -207,10 +207,19 @@ public void safeOperationComplete(int rc, Void result) {
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
- openComplete(BKException.Code.UnauthorizedAccessException, null);
+ openComplete(rc, null);
+ });
+ } else if (rc == BKException.Code.TimeoutException) {
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ if (ex != null) {
+ LOG.error("Ledger {} read timeout", ledgerId, ex);
+ }
+ openComplete(rc, null);
});
} else {
- openComplete(bk.getReturnRc(BKException.Code.LedgerRecoveryException), null);
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ openComplete(bk.getReturnRc(BKException.Code.LedgerRecoveryException), null);
+ });
}
}
Review Comment:
Already fixed
--
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] poorbarcode commented on pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#issuecomment-1289885952
Hi @hangc0276
> Would you please take a look at the failed CI?
Thanks for reminding me
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1000792663
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java:
##########
@@ -207,7 +207,14 @@ public void safeOperationComplete(int rc, Void result) {
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
- openComplete(BKException.Code.UnauthorizedAccessException, null);
+ openComplete(rc, null);
+ });
+ } else if (rc == BKException.Code.TimeoutException) {
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ if (ex != null) {
+ LOG.error("Ledger {} read timeout", ledgerId, ex);
+ }
+ openComplete(rc, null);
});
} else {
openComplete(bk.getReturnRc(BKException.Code.LedgerRecoveryException), null);
Review Comment:
Good suggestion. Already fixed
--
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 #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#issuecomment-1292861208
@poorbarcode Would you please rebase the master? thanks.
--
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] eolivelli commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1005474999
##########
pom.xml:
##########
@@ -673,6 +674,11 @@
<artifactId>junit</artifactId>
<version>${junit.version}</version>
</dependency>
+ <dependency>
+ <groupId>com.tngtech.junit.dataprovider</groupId>
Review Comment:
do we really need to add this new dependency ?
in junit4 there is support for parametrised tests
https://github.com/junit-team/junit4/wiki/Parameterized-tests
https://www.testwithspring.com/lesson/writing-parameterized-tests-with-junit-4/
--
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] poorbarcode commented on pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#issuecomment-1285521074
ping @zymap @dlg99 @eolivelli @hangc0276 @Shoothzj PTAL. Thanks.
--
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 #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#issuecomment-1293005185
@zymap Would you please help take a look? thanks.
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1005554159
##########
pom.xml:
##########
@@ -673,6 +674,11 @@
<artifactId>junit</artifactId>
<version>${junit.version}</version>
</dependency>
+ <dependency>
+ <groupId>com.tngtech.junit.dataprovider</groupId>
Review Comment:
Already move `OpenLedgerTest` to a new test class `BookKeeperBuildersOpenLedgerTest` and used feature `junit-parametrised`
--
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 #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#issuecomment-1289866132
@poorbarcode Would you please take a look at the failed CI?
https://github.com/apache/bookkeeper/actions/runs/3294663301/jobs/5433755230
--
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] codelipenghui commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1006369384
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -331,47 +330,6 @@ public void testOpenLedgerClientClosed() throws Exception {
.execute());
}
- @Test
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] zymap commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1006363403
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -331,47 +330,6 @@ public void testOpenLedgerClientClosed() throws Exception {
.execute());
}
- @Test
Review Comment:
Why remove them?
--
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] poorbarcode commented on pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#issuecomment-1292924095
rerun failure checks
--
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 #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
hangc0276 merged PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562
--
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] codelipenghui commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1000688470
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -25,6 +25,7 @@
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
+import static org.mockito.Mockito.*;
Review Comment:
Please avoid the star import.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -372,6 +375,61 @@ public void testOpenLedgerRecovery() throws Exception {
.execute());
}
+ @Test
+ public void testOpenLedgerNoRecoveryWithTimeoutEx() throws Exception {
+ mockReadEntryTimeout();
+ LedgerMetadata ledgerMetadata = generateLedgerMetadata(ensembleSize,
+ writeQuorumSize, ackQuorumSize, password, customMetadata);
+ registerMockLedgerMetadata(ledgerId, ledgerMetadata);
+ ledgerMetadata.getAllEnsembles().values().forEach(bookieAddressList -> {
+ bookieAddressList.forEach(bookieAddress -> {
+ registerMockEntryForRead(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED, bookieAddress, entryData, -1);
+ registerMockEntryForRead(ledgerId, 0, bookieAddress, entryData, -1);
+ });
+ });
+ try {
+ result(newOpenLedgerOp()
+ .withPassword(ledgerMetadata.getPassword())
+ .withDigestType(DigestType.CRC32)
+ .withLedgerId(ledgerId)
+ .withRecovery(false)
+ .execute());
+ fail("Expect timeout error");
+ } catch (BKException.BKTimeoutException timeoutException) {
+ // Expect timeout error.
+ }
+ // Reset bk client.
+ resetBKClient();
+ }
+
+ @Test
+ public void testOpenLedgerRecoveryWithTimeoutEx() throws Exception {
+ mockReadEntryTimeout();
+ LedgerMetadata ledgerMetadata = generateLedgerMetadata(ensembleSize,
+ writeQuorumSize, ackQuorumSize, password, customMetadata);
+ registerMockLedgerMetadata(ledgerId, ledgerMetadata);
+
+ ledgerMetadata.getAllEnsembles().values().forEach(bookieAddressList -> {
+ bookieAddressList.forEach(bookieAddress -> {
+ registerMockEntryForRead(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED, bookieAddress, entryData, -1);
+ registerMockEntryForRead(ledgerId, 0, bookieAddress, entryData, -1);
+ });
+ });
+ try {
+ result(newOpenLedgerOp()
+ .withPassword(ledgerMetadata.getPassword())
+ .withDigestType(DigestType.CRC32)
+ .withLedgerId(ledgerId)
+ .withRecovery(true)
+ .execute());
+ fail("Expect timeout error");
+ } catch (BKException.BKTimeoutException timeoutException) {
+ // Expect timeout error.
+ }
+ // Reset bk client.
+ resetBKClient();
+ }
Review Comment:
It's better to add a data provider to reduce the duplicated code.
--
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 #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1001329594
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java:
##########
@@ -207,10 +207,19 @@ public void safeOperationComplete(int rc, Void result) {
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
- openComplete(BKException.Code.UnauthorizedAccessException, null);
+ openComplete(rc, null);
+ });
+ } else if (rc == BKException.Code.TimeoutException) {
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ if (ex != null) {
+ LOG.error("Ledger {} read timeout", ledgerId, ex);
+ }
+ openComplete(rc, null);
});
} else {
- openComplete(bk.getReturnRc(BKException.Code.LedgerRecoveryException), null);
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ openComplete(bk.getReturnRc(BKException.Code.LedgerRecoveryException), null);
+ });
}
}
Review Comment:
also need log error:
```java
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java:
##########
@@ -207,10 +207,19 @@ public void safeOperationComplete(int rc, Void result) {
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
- openComplete(BKException.Code.UnauthorizedAccessException, null);
+ openComplete(rc, null);
+ });
+ } else if (rc == BKException.Code.TimeoutException) {
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ if (ex != null) {
+ LOG.error("Ledger {} read timeout", ledgerId, ex);
+ }
+ openComplete(rc, null);
});
Review Comment:
```java
LOG.error("Ledger {} read timeout", ledgerId, ex);
```
should be replaced with the following:
```java
LOG.error("Ledger {} close failed", ledgerId, ex);
```
the `ex` is not the read timeout exception.
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1000721347
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -372,6 +375,61 @@ public void testOpenLedgerRecovery() throws Exception {
.execute());
}
+ @Test
+ public void testOpenLedgerNoRecoveryWithTimeoutEx() throws Exception {
+ mockReadEntryTimeout();
+ LedgerMetadata ledgerMetadata = generateLedgerMetadata(ensembleSize,
+ writeQuorumSize, ackQuorumSize, password, customMetadata);
+ registerMockLedgerMetadata(ledgerId, ledgerMetadata);
+ ledgerMetadata.getAllEnsembles().values().forEach(bookieAddressList -> {
+ bookieAddressList.forEach(bookieAddress -> {
+ registerMockEntryForRead(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED, bookieAddress, entryData, -1);
+ registerMockEntryForRead(ledgerId, 0, bookieAddress, entryData, -1);
+ });
+ });
+ try {
+ result(newOpenLedgerOp()
+ .withPassword(ledgerMetadata.getPassword())
+ .withDigestType(DigestType.CRC32)
+ .withLedgerId(ledgerId)
+ .withRecovery(false)
+ .execute());
+ fail("Expect timeout error");
+ } catch (BKException.BKTimeoutException timeoutException) {
+ // Expect timeout error.
+ }
+ // Reset bk client.
+ resetBKClient();
+ }
+
+ @Test
+ public void testOpenLedgerRecoveryWithTimeoutEx() throws Exception {
+ mockReadEntryTimeout();
+ LedgerMetadata ledgerMetadata = generateLedgerMetadata(ensembleSize,
+ writeQuorumSize, ackQuorumSize, password, customMetadata);
+ registerMockLedgerMetadata(ledgerId, ledgerMetadata);
+
+ ledgerMetadata.getAllEnsembles().values().forEach(bookieAddressList -> {
+ bookieAddressList.forEach(bookieAddress -> {
+ registerMockEntryForRead(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED, bookieAddress, entryData, -1);
+ registerMockEntryForRead(ledgerId, 0, bookieAddress, entryData, -1);
+ });
+ });
+ try {
+ result(newOpenLedgerOp()
+ .withPassword(ledgerMetadata.getPassword())
+ .withDigestType(DigestType.CRC32)
+ .withLedgerId(ledgerId)
+ .withRecovery(true)
+ .execute());
+ fail("Expect timeout error");
+ } catch (BKException.BKTimeoutException timeoutException) {
+ // Expect timeout error.
+ }
+ // Reset bk client.
+ resetBKClient();
+ }
Review Comment:
Good idea
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1000747534
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -372,6 +375,61 @@ public void testOpenLedgerRecovery() throws Exception {
.execute());
}
+ @Test
+ public void testOpenLedgerNoRecoveryWithTimeoutEx() throws Exception {
+ mockReadEntryTimeout();
+ LedgerMetadata ledgerMetadata = generateLedgerMetadata(ensembleSize,
+ writeQuorumSize, ackQuorumSize, password, customMetadata);
+ registerMockLedgerMetadata(ledgerId, ledgerMetadata);
+ ledgerMetadata.getAllEnsembles().values().forEach(bookieAddressList -> {
+ bookieAddressList.forEach(bookieAddress -> {
+ registerMockEntryForRead(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED, bookieAddress, entryData, -1);
+ registerMockEntryForRead(ledgerId, 0, bookieAddress, entryData, -1);
+ });
+ });
+ try {
+ result(newOpenLedgerOp()
+ .withPassword(ledgerMetadata.getPassword())
+ .withDigestType(DigestType.CRC32)
+ .withLedgerId(ledgerId)
+ .withRecovery(false)
+ .execute());
+ fail("Expect timeout error");
+ } catch (BKException.BKTimeoutException timeoutException) {
+ // Expect timeout error.
+ }
+ // Reset bk client.
+ resetBKClient();
+ }
+
+ @Test
+ public void testOpenLedgerRecoveryWithTimeoutEx() throws Exception {
+ mockReadEntryTimeout();
+ LedgerMetadata ledgerMetadata = generateLedgerMetadata(ensembleSize,
+ writeQuorumSize, ackQuorumSize, password, customMetadata);
+ registerMockLedgerMetadata(ledgerId, ledgerMetadata);
+
+ ledgerMetadata.getAllEnsembles().values().forEach(bookieAddressList -> {
+ bookieAddressList.forEach(bookieAddress -> {
+ registerMockEntryForRead(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED, bookieAddress, entryData, -1);
+ registerMockEntryForRead(ledgerId, 0, bookieAddress, entryData, -1);
+ });
+ });
+ try {
+ result(newOpenLedgerOp()
+ .withPassword(ledgerMetadata.getPassword())
+ .withDigestType(DigestType.CRC32)
+ .withLedgerId(ledgerId)
+ .withRecovery(true)
+ .execute());
+ fail("Expect timeout error");
+ } catch (BKException.BKTimeoutException timeoutException) {
+ // Expect timeout error.
+ }
+ // Reset bk client.
+ resetBKClient();
+ }
Review Comment:
Already use `@DataProvider` to simpler the test case
--
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] poorbarcode commented on a diff in pull request #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1000746878
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/api/BookKeeperBuildersTest.java:
##########
@@ -25,6 +25,7 @@
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
+import static org.mockito.Mockito.*;
Review Comment:
Already remove the star import
--
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 #3562: When call openLedgerOp, make the timeout ex is a separate error code
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3562:
URL: https://github.com/apache/bookkeeper/pull/3562#discussion_r1000769912
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java:
##########
@@ -207,7 +207,14 @@ public void safeOperationComplete(int rc, Void result) {
if (ex != null) {
LOG.error("Ledger {} close failed", ledgerId, ex);
}
- openComplete(BKException.Code.UnauthorizedAccessException, null);
+ openComplete(rc, null);
+ });
+ } else if (rc == BKException.Code.TimeoutException) {
+ closeLedgerHandleAsync().whenComplete((r, ex) -> {
+ if (ex != null) {
+ LOG.error("Ledger {} read timeout", ledgerId, ex);
+ }
+ openComplete(rc, null);
});
} else {
openComplete(bk.getReturnRc(BKException.Code.LedgerRecoveryException), null);
Review Comment:
Do we need to close the ledgerHandle when encountering other exceptions?
--
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