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 2023/01/06 07:23:24 UTC
[GitHub] [bookkeeper] hangc0276 opened a new pull request, #3726: Add parameters check on ledger creation
hangc0276 opened a new pull request, #3726:
URL: https://github.com/apache/bookkeeper/pull/3726
### Motivation
When the BookKeeper client creates a ledger with illegal parameters, such as `EnsembleSize = 0`, `WriteQuorumSize = 0`, and `AckQuorumSize = 0`, the ledger can be created successfully. However, when we use the ledgerHandle to write entries into this ledger, the write operation will be blocked without any logs or exceptions. It will be hard to debug.
### Modification
Add parameters check on ledger creation.
--
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 #3726: Add parameters check on ledger creation
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1110964733
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1058,9 +1056,7 @@ public LedgerHandle createLedgerAdv(int ensSize, int writeQuorumSize, int ackQuo
public void asyncCreateLedgerAdv(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd, final CreateCallback cb, final Object ctx,
final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
Review Comment:
Make sense.
--
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 #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1063213234
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1663,4 +1657,17 @@ public ByteBufAllocator getByteBufAllocator() {
public ClientContext getClientCtx() {
return clientCtx;
}
+
+ private void checkLedgerCreationParameters(int ensSize, int writeQuorumSize, int ackQuorumSize) {
+ if (ensSize <= 0
+ || writeQuorumSize <= 0
+ || ackQuorumSize <= 0
Review Comment:
I think `ackQuorumSize==0` is legal, which means no need ack; just write.
--
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 #3726: Add parameters check on ledger creation
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1110975783
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java:
##########
@@ -1127,4 +1132,87 @@ public void testBookieAddressResolverPassedToDNSToSwitchMapping() throws Excepti
}
}
+ @DataProvider
+ public static Object[][] IllegalLedgerCreationArguments() {
+ return new Object[][] {
+ {0, 0, 0},
+ {3, 3, -1},
+ {2, 3, 1},
+ {2, 2, 3},
+ {3, 3, 0}
+ };
+ }
+ @Test
+ @UseDataProvider("IllegalLedgerCreationArguments")
+ public void testCreateLedgerWithIllegalArguments(int ensSize, int writeQuorumSize, int ackQuorumSize) throws Exception {
+ final byte[] passwd = "testpasswd".getBytes();
+ final byte[] data = "data".getBytes();
+
+ try {
+ LedgerHandle lh = bkc.createLedger(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
Review Comment:
The dead code can be removed.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java:
##########
@@ -1127,4 +1132,87 @@ public void testBookieAddressResolverPassedToDNSToSwitchMapping() throws Excepti
}
}
+ @DataProvider
+ public static Object[][] IllegalLedgerCreationArguments() {
+ return new Object[][] {
+ {0, 0, 0},
+ {3, 3, -1},
+ {2, 3, 1},
+ {2, 2, 3},
+ {3, 3, 0}
+ };
+ }
+ @Test
+ @UseDataProvider("IllegalLedgerCreationArguments")
+ public void testCreateLedgerWithIllegalArguments(int ensSize, int writeQuorumSize, int ackQuorumSize) throws Exception {
+ final byte[] passwd = "testpasswd".getBytes();
+ final byte[] data = "data".getBytes();
+
+ try {
+ LedgerHandle lh = bkc.createLedger(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
+ lh.addEntry(data);
+ }
+ lh.close();
+ fail();
+ } catch (Exception e) {
+ assertTrue(e instanceof BKException.BKIncorrectParameterException);
+ assertEquals(e.getMessage(), "Incorrect parameter input");
+ }
+
+ try {
+ LedgerHandle lh = bkc.createLedgerAdv(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
Review Comment:
The dead code can be removed.
--
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 #3726: Add parameters check on ledger creation
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1110502184
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1058,9 +1056,7 @@ public LedgerHandle createLedgerAdv(int ensSize, int writeQuorumSize, int ackQuo
public void asyncCreateLedgerAdv(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd, final CreateCallback cb, final Object ctx,
final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
Review Comment:
It can avoid the LedgerCreateOp objects creation if the parameters are illegal.
--
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 #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1063216808
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1663,4 +1657,17 @@ public ByteBufAllocator getByteBufAllocator() {
public ClientContext getClientCtx() {
return clientCtx;
}
+
+ private void checkLedgerCreationParameters(int ensSize, int writeQuorumSize, int ackQuorumSize) {
+ if (ensSize <= 0
+ || writeQuorumSize <= 0
+ || ackQuorumSize <= 0
Review Comment:
I check again, `ackRuorumSize == 0` is meaningless for us. The `PendingAddOp#writeComplete` need to callback at least once. We didn't complete the `PendingAddOp` when the `ackRuorumSize == 0`.
So keep it as it is
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1663,4 +1657,17 @@ public ByteBufAllocator getByteBufAllocator() {
public ClientContext getClientCtx() {
return clientCtx;
}
+
+ private void checkLedgerCreationParameters(int ensSize, int writeQuorumSize, int ackQuorumSize) {
+ if (ensSize <= 0
+ || writeQuorumSize <= 0
+ || ackQuorumSize <= 0
Review Comment:
~I think `ackQuorumSize==0` is legal, which means no need ack; just write.~
--
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 #3726: Add parameters check on ledger creation
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1111045293
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java:
##########
@@ -1127,4 +1132,87 @@ public void testBookieAddressResolverPassedToDNSToSwitchMapping() throws Excepti
}
}
+ @DataProvider
+ public static Object[][] IllegalLedgerCreationArguments() {
+ return new Object[][] {
+ {0, 0, 0},
+ {3, 3, -1},
+ {2, 3, 1},
+ {2, 2, 3},
+ {3, 3, 0}
+ };
+ }
+ @Test
+ @UseDataProvider("IllegalLedgerCreationArguments")
+ public void testCreateLedgerWithIllegalArguments(int ensSize, int writeQuorumSize, int ackQuorumSize) throws Exception {
+ final byte[] passwd = "testpasswd".getBytes();
+ final byte[] data = "data".getBytes();
+
+ try {
+ LedgerHandle lh = bkc.createLedger(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
+ lh.addEntry(data);
+ }
+ lh.close();
+ fail();
+ } catch (Exception e) {
+ assertTrue(e instanceof BKException.BKIncorrectParameterException);
+ assertEquals(e.getMessage(), "Incorrect parameter input");
+ }
+
+ try {
+ LedgerHandle lh = bkc.createLedgerAdv(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
Review Comment:
We'd better not remove the addEntry code, because create Ledger will succeed and the addEntry will be blocked without this PR
--
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 #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1063216808
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1663,4 +1657,17 @@ public ByteBufAllocator getByteBufAllocator() {
public ClientContext getClientCtx() {
return clientCtx;
}
+
+ private void checkLedgerCreationParameters(int ensSize, int writeQuorumSize, int ackQuorumSize) {
+ if (ensSize <= 0
+ || writeQuorumSize <= 0
+ || ackQuorumSize <= 0
Review Comment:
I check again, `ackRuorumSize == 0` is meaningless for us. The `PendingAddOp#writeComplete` need to callback at least once. We didn't complete the `PendingAddOp` directly when the `ackRuorumSize == 0`.
So keep it as it is
--
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 #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1064328088
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java:
##########
@@ -1127,4 +1127,193 @@ public void testBookieAddressResolverPassedToDNSToSwitchMapping() throws Excepti
}
}
+ @Test
+ public void testCreateLedgerWithIllegalArguments() throws Exception {
Review Comment:
You can use data provider to define the different case of the test.
--
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 #3726: Add parameters check on ledger creation
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1111051186
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java:
##########
@@ -1127,4 +1132,87 @@ public void testBookieAddressResolverPassedToDNSToSwitchMapping() throws Excepti
}
}
+ @DataProvider
+ public static Object[][] IllegalLedgerCreationArguments() {
+ return new Object[][] {
+ {0, 0, 0},
+ {3, 3, -1},
+ {2, 3, 1},
+ {2, 2, 3},
+ {3, 3, 0}
+ };
+ }
+ @Test
+ @UseDataProvider("IllegalLedgerCreationArguments")
+ public void testCreateLedgerWithIllegalArguments(int ensSize, int writeQuorumSize, int ackQuorumSize) throws Exception {
+ final byte[] passwd = "testpasswd".getBytes();
+ final byte[] data = "data".getBytes();
+
+ try {
+ LedgerHandle lh = bkc.createLedger(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
+ lh.addEntry(data);
+ }
+ lh.close();
+ fail();
+ } catch (Exception e) {
+ assertTrue(e instanceof BKException.BKIncorrectParameterException);
+ assertEquals(e.getMessage(), "Incorrect parameter input");
+ }
+
+ try {
+ LedgerHandle lh = bkc.createLedgerAdv(ensSize, writeQuorumSize, ackQuorumSize,
+ BookKeeper.DigestType.CRC32, passwd);
+ for (int i = 0; i < 10; ++i) {
Review Comment:
Make sense.
--
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 #3726: Add parameters check on ledger creation
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1136654262
##########
pom.xml:
##########
@@ -696,6 +697,11 @@
<artifactId>junit</artifactId>
<version>${junit.version}</version>
</dependency>
+ <dependency>
+ <groupId>com.tngtech.java</groupId>
Review Comment:
usually it is better to not add such third party dependencies (tngtech ??) if not strictly needed
in JUnit4 this is the way to do it
https://github.com/junit-team/junit4/wiki/Parameterized-tests
--
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] StevenLuMT commented on a diff in pull request #3726: Add parameters check on ledger creation
Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1089677438
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -859,9 +859,7 @@ public void asyncCreateLedger(final int ensSize,
public void asyncCreateLedger(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd,
final CreateCallback cb, final Object ctx, final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
Review Comment:
same question
--
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 pull request #3726: Add parameters check on ledger creation
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#issuecomment-1437932393
We added more limitations on creating a ledger. May introduce some break changes? Remove the minor release tag.
--
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 #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#issuecomment-1373252761
@horizonzy Please help take a look at this PR, 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] horizonzy commented on a diff in pull request #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1063220006
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -859,9 +859,7 @@ public void asyncCreateLedger(final int ensSize,
public void asyncCreateLedger(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd,
final CreateCallback cb, final Object ctx, final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
Review Comment:
It will throw exceptions directly, the user may need to try catch `asyncCreateLedger`.
How about `cb.createComplete` with an error 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] horizonzy commented on a diff in pull request #3726: Add parameters check on ledger creation
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1063221318
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1058,9 +1056,7 @@ public LedgerHandle createLedgerAdv(int ensSize, int writeQuorumSize, int ackQuo
public void asyncCreateLedgerAdv(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd, final CreateCallback cb, final Object ctx,
final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
Review Comment:
Shall move the check logic to `LedgerCreateOp#initiate`; that's the only one entrace.
--
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] codecov-commenter commented on pull request #3726: Add parameters check on ledger creation
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#issuecomment-1435697021
# [Codecov](https://codecov.io/gh/apache/bookkeeper/pull/3726?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@405e72a`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #3726 +/- ##
=========================================
Coverage ? 21.01%
Complexity ? 2015
=========================================
Files ? 473
Lines ? 40965
Branches ? 5238
=========================================
Hits ? 8609
Misses ? 31081
Partials ? 1275
```
| Flag | Coverage Δ | |
|---|---|---|
| tls | `21.01% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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 #3726: Add parameters check on ledger creation
Posted by "wenbingshen (via GitHub)" <gi...@apache.org>.
wenbingshen commented on code in PR #3726:
URL: https://github.com/apache/bookkeeper/pull/3726#discussion_r1090546251
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1663,4 +1657,17 @@ public ByteBufAllocator getByteBufAllocator() {
public ClientContext getClientCtx() {
return clientCtx;
}
+
+ private void checkLedgerCreationParameters(int ensSize, int writeQuorumSize, int ackQuorumSize) {
+ if (ensSize <= 0
+ || writeQuorumSize <= 0
+ || ackQuorumSize < 0
+ || writeQuorumSize > ensSize
+ || ackQuorumSize > writeQuorumSize) {
+ LOG.error("Illegal parameter: ensembleSize: {}, writeQuorumSize: {}, ackQuorumSize: {}",
Review Comment:
Illegal parameters
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -1058,9 +1056,7 @@ public LedgerHandle createLedgerAdv(int ensSize, int writeQuorumSize, int ackQuo
public void asyncCreateLedgerAdv(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd, final CreateCallback cb, final Object ctx,
final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
Review Comment:
+1
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java:
##########
@@ -1127,4 +1127,193 @@ public void testBookieAddressResolverPassedToDNSToSwitchMapping() throws Excepti
}
}
+ @Test
+ public void testCreateLedgerWithIllegalArguments() throws Exception {
Review Comment:
+1
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -859,9 +859,7 @@ public void asyncCreateLedger(final int ensSize,
public void asyncCreateLedger(final int ensSize, final int writeQuorumSize, final int ackQuorumSize,
final DigestType digestType, final byte[] passwd,
final CreateCallback cb, final Object ctx, final Map<String, byte[]> customMetadata) {
- if (writeQuorumSize < ackQuorumSize) {
- throw new IllegalArgumentException("Write quorum must be larger than ack quorum");
- }
+ checkLedgerCreationParameters(ensSize, writeQuorumSize, ackQuorumSize);
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