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/09/26 17:45:05 UTC
[GitHub] [bookkeeper] wenbingshen opened a new pull request, #3505: update success about wait writeSet for Writable result
wenbingshen opened a new pull request, #3505:
URL: https://github.com/apache/bookkeeper/pull/3505
### Changes
update `success` about wait writeSet for Writable result.
--
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 merged pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
zymap merged PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505
--
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 pull request #3505: update success about wait writeSet for Writable result
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#issuecomment-1258396555
ping @dlg99 @hangc0276 @Shoothzj @eolivelli @StevenLuMT 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] zymap commented on a diff in pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#discussion_r998056620
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java:
##########
@@ -390,4 +390,43 @@ public void operationComplete(int rc, Set<LedgerFragment> fragments) {
checklatch.await();
assertEquals("There should be no missing fragments", 0, numFragments.get());
}
+
+ @Test
+ public void testWaitForWritable() throws Exception {
+ final ClientConfiguration conf = new ClientConfiguration();
+ conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
+ BookKeeper bkc = new BookKeeper(conf);
+
+ byte[] pwd = new byte[]{};
+ try (LedgerHandle lh = bkc.createLedger(1, 1, 1, BookKeeper.DigestType.CRC32, pwd)) {
+ long entryId = lh.addEntry(this.entry);
+
+ RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(1, 1, 1);
+ DistributionSchedule.WriteSet writeSet = schedule.getWriteSet(entryId);
+
+ int slowBookieIndex = writeSet.get(ThreadLocalRandom.current().nextInt(writeSet.size()));
+ List<BookieId> curEns = lh.getCurrentEnsemble();
+
+ // disable channel writable
+ setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, false);
+
+ final CountDownLatch cdl = new CountDownLatch(1);
+ AtomicBoolean isWriteable = new AtomicBoolean(false);
+ final long timeout = 10000;
+
+ // waitForWritable async
+ new Thread(() -> {
+ isWriteable.set(lh.waitForWritable(writeSet, 0, timeout));
+ cdl.countDown();
+ }).start();
+
+ // enable channel writable
+ setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, true);
+
+ cdl.await(timeout, TimeUnit.MILLISECONDS);
+
+ assertTrue("We should check b0 is writeable", isWriteable.get());
Review Comment:
```suggestion
new Thread(() -> {
isWriteable.set(lh.waitForWritable(writeSet, 0, timeout));
}).start();
TimeUnit.MILLISECONDS.sleep(5000);
assertFalse(isWriteable.get());
setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, true);
Awaitility.await().untilAsserted(() -> assertTrue(isWriteable.get()));
```
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java:
##########
@@ -390,4 +390,43 @@ public void operationComplete(int rc, Set<LedgerFragment> fragments) {
checklatch.await();
assertEquals("There should be no missing fragments", 0, numFragments.get());
}
+
+ @Test
+ public void testWaitForWritable() throws Exception {
+ final ClientConfiguration conf = new ClientConfiguration();
+ conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
+ BookKeeper bkc = new BookKeeper(conf);
+
+ byte[] pwd = new byte[]{};
+ try (LedgerHandle lh = bkc.createLedger(1, 1, 1, BookKeeper.DigestType.CRC32, pwd)) {
+ long entryId = lh.addEntry(this.entry);
+
+ RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(1, 1, 1);
+ DistributionSchedule.WriteSet writeSet = schedule.getWriteSet(entryId);
+
+ int slowBookieIndex = writeSet.get(ThreadLocalRandom.current().nextInt(writeSet.size()));
+ List<BookieId> curEns = lh.getCurrentEnsemble();
+
+ // disable channel writable
+ setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, false);
+
+ final CountDownLatch cdl = new CountDownLatch(1);
Review Comment:
```suggestion
```
--
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 #3505: update success about wait writeSet for Writable result
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#discussion_r980687850
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##########
@@ -1262,7 +1262,7 @@ protected boolean waitForWritable(DistributionSchedule.WriteSet writeSet,
final int maxBackoff = 4;
final long deadline = startTime + TimeUnit.MILLISECONDS.toNanos(durationMs);
- while (!isWriteSetWritable(writeSet, allowedNonWritableCount)) {
+ while (!(success = isWriteSetWritable(writeSet, allowedNonWritableCount))) {
Review Comment:
I think success can be named writableResult,like the code:
`while (!(writableResult = isWriteSetWritable(writeSet, allowedNonWritableCount))) {`
--
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 pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#issuecomment-1282267943
> Sorry for another minor comment. I just want to make sure the test can not be passed without this change.
I copied this test and run it locally, and it passed. I left a comment about how to fix it. PTAL. :)
@zymap Good. Thanks for your comments. I have committed it. 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] wenbingshen commented on a diff in pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#discussion_r983191361
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java:
##########
@@ -390,4 +390,32 @@ public void operationComplete(int rc, Set<LedgerFragment> fragments) {
checklatch.await();
assertEquals("There should be no missing fragments", 0, numFragments.get());
}
+
+ @Test
+ public void testWaitForWritable() throws Exception {
+ final ClientConfiguration conf = new ClientConfiguration();
+ conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
+ BookKeeper bkc = new BookKeeper(conf);
+
+ byte[] pwd = new byte[]{};
+ try (LedgerHandle lh = bkc.createLedger(1, 1, 1, BookKeeper.DigestType.CRC32, pwd)) {
+ long entryId = lh.addEntry(this.entry);
+
+ RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(1, 1, 1);
+ DistributionSchedule.WriteSet writeSet = schedule.getWriteSet(entryId);
+
+ int slowBookieIndex = writeSet.get(ThreadLocalRandom.current().nextInt(writeSet.size()));
+ List<BookieId> curEns = lh.getCurrentEnsemble();
+
+ // disable channel writable
+ setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, false);
+ boolean isWriteable = lh.waitForWritable(writeSet, 0, 1000);
Review Comment:
ok, i'll fix it later.
--
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 pull request #3505: update success about wait writeSet for Writable result
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#issuecomment-1259189064
> Great! Maybe we need to add a unit test?
@zymap I have add a unit test. 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] wenbingshen commented on pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#issuecomment-1283404100
@zymap All tests passed. Can this pr be merged? 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] zymap commented on a diff in pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#discussion_r981921160
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java:
##########
@@ -390,4 +390,32 @@ public void operationComplete(int rc, Set<LedgerFragment> fragments) {
checklatch.await();
assertEquals("There should be no missing fragments", 0, numFragments.get());
}
+
+ @Test
+ public void testWaitForWritable() throws Exception {
+ final ClientConfiguration conf = new ClientConfiguration();
+ conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
+ BookKeeper bkc = new BookKeeper(conf);
+
+ byte[] pwd = new byte[]{};
+ try (LedgerHandle lh = bkc.createLedger(1, 1, 1, BookKeeper.DigestType.CRC32, pwd)) {
+ long entryId = lh.addEntry(this.entry);
+
+ RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(1, 1, 1);
+ DistributionSchedule.WriteSet writeSet = schedule.getWriteSet(entryId);
+
+ int slowBookieIndex = writeSet.get(ThreadLocalRandom.current().nextInt(writeSet.size()));
+ List<BookieId> curEns = lh.getCurrentEnsemble();
+
+ // disable channel writable
+ setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, false);
+ boolean isWriteable = lh.waitForWritable(writeSet, 0, 1000);
Review Comment:
This is always true/false if you set the state before you call this method. We need a test to make sure after the backoff retries, it returns the correct state after the `waitForWritable`.
So the test case will be like this:
1. change the writable to false.
2. call the `waitForWriable`, it will retry with backoff.
3. at the same time, change the writable to true.
4. we should get the true from the step2 call.
--
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 #3505: update success about wait writeSet for Writable result
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#discussion_r980956726
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##########
@@ -1262,7 +1262,7 @@ protected boolean waitForWritable(DistributionSchedule.WriteSet writeSet,
final int maxBackoff = 4;
final long deadline = startTime + TimeUnit.MILLISECONDS.toNanos(durationMs);
- while (!isWriteSetWritable(writeSet, allowedNonWritableCount)) {
+ while (!(success = isWriteSetWritable(writeSet, allowedNonWritableCount))) {
Review Comment:
Thanks. I have addressed your comment.
--
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 pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#issuecomment-1259267957
> Are you only renaming a variable?
No, i updated the title.
> I think that adding a test is the maim content of this patch. Maybe we should change the title
The code before this PR is like this:
```java
while (!isWriteSetWritable(writeSet, allowedNonWritableCount)) {
...
}
```
The code after this PR is this:
```java
while (!(writableResult = isWriteSetWritable(writeSet, allowedNonWritableCount))) {
...
}
```
writableResult will not update the last result, but this result needs to be used later, we should update 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
[GitHub] [bookkeeper] wenbingshen commented on a diff in pull request #3505: Rename success with writableResult and update final writableResult about wait writeSet
Posted by GitBox <gi...@apache.org>.
wenbingshen commented on code in PR #3505:
URL: https://github.com/apache/bookkeeper/pull/3505#discussion_r996592651
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java:
##########
@@ -390,4 +390,32 @@ public void operationComplete(int rc, Set<LedgerFragment> fragments) {
checklatch.await();
assertEquals("There should be no missing fragments", 0, numFragments.get());
}
+
+ @Test
+ public void testWaitForWritable() throws Exception {
+ final ClientConfiguration conf = new ClientConfiguration();
+ conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
+ BookKeeper bkc = new BookKeeper(conf);
+
+ byte[] pwd = new byte[]{};
+ try (LedgerHandle lh = bkc.createLedger(1, 1, 1, BookKeeper.DigestType.CRC32, pwd)) {
+ long entryId = lh.addEntry(this.entry);
+
+ RoundRobinDistributionSchedule schedule = new RoundRobinDistributionSchedule(1, 1, 1);
+ DistributionSchedule.WriteSet writeSet = schedule.getWriteSet(entryId);
+
+ int slowBookieIndex = writeSet.get(ThreadLocalRandom.current().nextInt(writeSet.size()));
+ List<BookieId> curEns = lh.getCurrentEnsemble();
+
+ // disable channel writable
+ setTargetChannelState(bkc, curEns.get(slowBookieIndex), 0, false);
+ boolean isWriteable = lh.waitForWritable(writeSet, 0, 1000);
Review Comment:
@zymap I have addressed your comments. 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