You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/11/08 11:08:08 UTC
[GitHub] [pulsar] thetumbled opened a new pull request, #18390: [fix][broker] fix bug caused by optimistic locking
thetumbled opened a new pull request, #18390:
URL: https://github.com/apache/pulsar/pull/18390
<!-- Either this PR fixes an issue, -->
Fixes #18388
### Motivation
ConcurrentLongLongPairHashMap use optimistic locking to read data in Section, but if there is another thread that is removing data from the same Section, and trigger the shrink process, then the index calculated basing on dirty capacity exceed the array size, an ArrayIndexOut0fBoundsException will be throw, which is not handled now. Eventually the connection with client will be closed.
### Modifications
just catch the exception. If any thread reads a dirty capacity, if will fall back to pessimistic reading.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as *(please describe tests)*.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1336703924
> @thetumbled i think this fix works, can you add some concurrent test on this case?
we have deployed the patch to production pulsar cluster for almost one month, which work pretty well.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1051770521
##########
pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMapTest.java:
##########
@@ -173,6 +174,62 @@ public void testExpandAndShrink() {
assertEquals(map.capacity(), 8);
}
+ @Test
+ public void testConcurrentExpandAndShrinkAndGet() throws Throwable {
+ ConcurrentLongLongPairHashMap map = ConcurrentLongLongPairHashMap.newBuilder()
+ .expectedItems(2)
+ .concurrencyLevel(1)
+ .autoShrink(true)
+ .mapIdleFactor(0.25f)
+ .build();
+ assertEquals(map.capacity(), 4);
+
+ ExecutorService executor = Executors.newCachedThreadPool();
+ final int readThreads = 16;
+ final int writeThreads = 1;
Review Comment:
The test name seems to be a concurrent test, but why do we set only 1 write thread? Same question about other 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1019133140
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,24 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
As long as we use optimistic read, we can't predict when the shrinking process triggered. it seems that we can only use try/catch block to surround the array accessing code.
In fact, we can predict the results of code execution.
It is worth noting that following code is not only used for optimistic reading, but also used for pessimistic reading.
```
try {
storedKey1 = table[bucket];
storedKey2 = table[bucket + 1];
storedValue1 = table[bucket + 2];
storedValue2 = table[bucket + 3];
} catch (ArrayIndexOutOfBoundsException e) {
```
First, execute optimistic reading, that is, execute the above code. After reading the array content, execute the following code block.
```
if (!acquiredLock && validate(stamp)) {
// The values we have read are consistent
if (key1 == storedKey1 && key2 == storedKey2) {
return new LongPair(storedValue1, storedValue2);
} else if (storedKey1 == EmptyKey) {
// Not found
return null;
}
}
```
At some time, other thread acquire the write lock, we may catch `ArrayIndexOutOfBoundsException` and then execute the following code block **for only once**. When acquiring the read lock for the first time, we need to recalculate the initial value of the traversal bucket, because the capacity may have changed.
```
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
**bucket = signSafeMod(keyHash, capacity);**
storedKey1 = table[bucket];
storedKey2 = table[bucket + 1];
storedValue1 = table[bucket + 2];
storedValue2 = table[bucket + 3];
}
```
After that, the buckets will be traversed **using pessimistic reading**. The loop will terminate only when the bucket encounters the same `(key1, key2)` or `EmptyKey`. Because we have acquired pessimistic read lock, we can ensure that there is no any concurrent problem.
I don't think that there are any unpredictable cases with these 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] WJL3333 commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
WJL3333 commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1019868610
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,24 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
i think we need fix the lock problem，catch exception is just an workaround for this 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1309899216
> Nice catch! And I think the `ConcurrentLongLongPairHashMap.Section#forEach` method may have the same problem. Could you help to confirm it?
>
> 
i find that, even if there is any thread acquired write lock and trigger shrink/expand process, the local variable `table` will not be updated, so the index used in loop will not exceed the bound.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1352628072
> For the test case, maybe this can help:
>
> https://github.com/apache/bookkeeper/blob/43bdfb35317d9a69840d17df94c5c801a4cbba62/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashMapTest.java#L424-L489
It is also difficult to reproduce concurrency problems using the following code for stress testing.
```
@Test
public void testExpandAndShrinkAndGet() throws Throwable {
ConcurrentLongLongPairHashMap map = ConcurrentLongLongPairHashMap.newBuilder()
.expectedItems(2)
.concurrencyLevel(1)
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
assertEquals(map.capacity(), 4);
ExecutorService executor = Executors.newCachedThreadPool();
final int readThreads = 16;
final int writeThreads = 1;
final int n = 1_000_000;
CyclicBarrier barrier = new CyclicBarrier(writeThreads + readThreads);
Future<?> future = null;
for (int i = 0; i < readThreads; i++) {
executor.submit(() -> {
try {
barrier.await();
} catch (Exception e) {
throw new RuntimeException(e);
}
while (true) {
map.get(1, 1);
}
});
}
assertTrue(map.put(1, 1, 11, 11));
future = executor.submit(() -> {
try {
barrier.await();
} catch (Exception e) {
throw new RuntimeException(e);
}
for (int i = 0; i < n; i++) {
// expand hashmap
assertTrue(map.put(2, 2, 22, 22));
assertTrue(map.put(3, 3, 33, 33));
assertEquals(map.capacity(), 8);
// shrink hashmap
assertTrue(map.remove(3, 3, 33, 33));
assertTrue(map.remove(2, 2, 22, 22));
assertEquals(map.capacity(), 4);
}
});
future.get();
// shut down pool
executor.shutdown();
}
```
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1700791072
I have removed my "request changes" in order to unblock the patch
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312669549
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +325,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / ITEM_SIZE);
Review Comment:
I'll create a separate PR to fix that issue. The main impact is non-optimal lookup since hash collisions will happen when they shouldn't.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1700789600
@merlimat @lhotari can you please help review ?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1718819835
/pulsarbot 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1352623307
if we can modify the implementation of `ConcurrentLongLongPairHashMap.Section#get`, that is to insert following code
`Thread.sleep(3000);` between arrays access.
<img width="432" alt="image" src="https://user-images.githubusercontent.com/52550727/207790130-eb81eed5-7b95-4150-b760-3db4326f01c5.png">
then we can reproduce concurrent problem stably using following test code.
```
@Test
public void testExpandAndShrinkAndGet2() throws Throwable {
ConcurrentLongLongPairHashMap map = ConcurrentLongLongPairHashMap.newBuilder()
.expectedItems(2)
.concurrencyLevel(1)
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
assertEquals(map.capacity(), 4);
ExecutorService executor = Executors.newCachedThreadPool();
// expand hashmap
assertTrue(map.put(1, 1, 11, 11));
assertTrue(map.put(2, 2, 22, 22));
assertTrue(map.put(3, 3, 33, 33));
assertEquals(map.capacity(), 8);
Future<?> future = null;
future = executor.submit(() -> {
map.get(1, 1);
});
// shrink hashmap
assertTrue(map.remove(2, 2, 22, 22));
Thread.sleep(1000);
assertTrue(map.remove(3, 3, 33, 33));
assertEquals(map.capacity(), 4);
future.get();
executor.shutdown();
}
```
<img width="1355" alt="image" src="https://user-images.githubusercontent.com/52550727/207790378-07049b45-0510-4b02-a929-e3c8fd0563f4.png">
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1348461175
> @thetumbled i think this fix works, can you add some concurrent test on this case?
Do you have any idea about how to implement the test code? It's difficult to reproduce concurrency problems in test code.
But we have deployed the patch to production pulsar cluster for over a month, which work pretty well.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1311085034
Yes, `table == this.table` seems not necessary. So we can choose one solution from these two approaches.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liangyepianzhou commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
liangyepianzhou commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1309660446
And `ConcurrentLongLongPairHashMap.Section#remove` has the same issue. Please also handle 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1309713362
> And `ConcurrentLongLongPairHashMap.Section#remove` has the same issue. Please also handle it.
i don't find that `ConcurrentLongLongPairHashMap.Section#remove` do use any optimistic lock but write lock.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1307033065
@thetumbled Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1308100085
> But I'm still not sure about the following logic
>
> ```java
> // First try optimistic locking
> long storedKey1 = table[bucket];
> long storedKey2 = table[bucket + 1];
> long storedValue1 = table[bucket + 2];
> long storedValue2 = table[bucket + 3];
>
> if (!acquiredLock && validate(stamp)) {
> // The values we have read are consistent
> if (key1 == storedKey1 && key2 == storedKey2) {
> return new LongPair(storedValue1, storedValue2);
> ```
>
> Even if storedKey1 and storeKey2 matched the expected keys, are storedValue1 and storedValue2 guaranteed to be exactly the associated values? This PR covered the case that table is shrinking, which makes the index invalid. However, could it happen that when retrieving `table[bucket + 2]` and `table[bucket + 3]`, these values have been modified? @lhotari @lordcheng10 @thetumbled
if `table[bucket + 2]` and `table[bucket + 3]` is modified, there must be one thread having acquired write lock, which result into that `validate(stamp)` return false and fallback to pessimistic read.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1702123088
> Great work @thetumbled ! Please check the few minor details that I provided as review comments.
Thanks a lot. I have updated to fix 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1323875177
##########
pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMapTest.java:
##########
@@ -213,6 +214,66 @@ public void testExpandShrinkAndClear() {
assertTrue(map.capacity() == initCapacity);
}
+ @Test
Review Comment:
done.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1354080893
These maps or sets all have the same risk： @thetumbled
org.apache.pulsar.common.util.collections.ConcurrentLongPairSet
org.apache.pulsar.common.util.collections.ConcurrentOpenHashMap
org.apache.pulsar.common.util.collections.ConcurrentOpenHashSet
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1336703658
>
we have deploy the patch to production pulsar cluster for almost one month, which work pretty well.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310328192
Oh I see, thanks for your explanation.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310527633
>
It can also fix the bug. But is `&& table == this.table` indispensable? If `table != this.table` ,there must be other thread acquire write lock and change `this.table`, which make `validate(stamp)` return false.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1350268043
nice catch!
I left some comments. @thetumbled
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1351461673
For the test case, maybe this can help:
https://github.com/apache/bookkeeper/blob/43bdfb35317d9a69840d17df94c5c801a4cbba62/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashMapTest.java#L424-L489
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1350304181
> @thetumbled i think this fix works, can you add some concurrent test on this case?
+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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312630952
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, values.length);
storedKey = keys[bucket];
storedValue = values[bucket];
}
- if (capacity != this.capacity) {
- // There has been a rehashing. We need to restart the search
- bucket = keyHash;
- continue;
- }
-
if (storedKey == key) {
return storedValue != DeletedValue ? storedValue : null;
} else if (storedValue == EmptyValue) {
// Not found
return null;
}
}
-
- ++bucket;
+ bucket = (bucket + 1) & (values.length - 1);
Review Comment:
We iterate the array here. As the length of array is an exponential power of 2, `&(values.length - 1)` is equal to `mod values.length`.
We only need to use `signSafeMod` for initialization of `bucket`.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312636778
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -361,7 +368,7 @@ LongPair get(long key1, long key2, int keyHash) {
}
}
- bucket = (bucket + 4) & (table.length - 1);
Review Comment:
The original code seems strange to me.
* Why is there a bitwise AND operation?
It looks like a bug. modulus seems to make more sense. @merlimat could you please check?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312662773
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -361,7 +368,7 @@ LongPair get(long key1, long key2, int keyHash) {
}
}
- bucket = (bucket + 4) & (table.length - 1);
Review Comment:
After checking, it is a power of 2 in the locations where the optimization is used, so no need to make changes.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1322969520
##########
pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMapTest.java:
##########
@@ -213,6 +214,66 @@ public void testExpandShrinkAndClear() {
assertTrue(map.capacity() == initCapacity);
}
+ @Test
Review Comment:
```suggestion
@Test(invocationCount = 10)
```
It's better to add `invocationCount` to avoid regression because even if new changes breaks the behavior, the test might also get passed.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1016590222
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,19 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ // read a dirty capacity, cause ArrayIndexOutOfBoundsException
+ logger.error("Read a dirty capacity, fallback to Pessimistic Read. capacity:{}, bucket:{}", capacity, bucket);
Review Comment:
logging with error level is not very useful since this is an expected condition. Using debug level would be justified.
It could make sense to also log `e.getMessage()`.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1019046327
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,24 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
I don't think this is a good fix.
It is unpredictable
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310528449
> > In the `rehash` method, the member `this.table` of `Section` will be reallocated a new long[] array, which is different from old table member.
> > ```
> > private void rehash(int newCapacity) {
> > long[] newTable = new long[4 * newCapacity];
> > ...
> > table = newTable;
> > ...
> > }
> > ```
>
> This inspires me. Can we fix the `get` issue in the same way?
>
> ```java
> boolean acquiredLock = false;
> // add local variable here, so OutOfBound won't happen
> long[] table = this.table;
> // calculate table.length / 4 as capacity to avoid rehash changing capacity
> int bucket = signSafeMod(keyHash, table.length / 4);
>
> try {
> while (true) {
> long storedKey1 = table[bucket];
> long storedKey2 = table[bucket + 1];
> long storedValue1 = table[bucket + 2];
> long storedValue2 = table[bucket + 3];
>
> // compare table to ensure not rehash
> if (!acquiredLock && validate(stamp) && table == this.table) {
> if (key1 == storedKey1 && key2 == storedKey2) {
> return new LongPair(storedValue1, storedValue2);
> } else if (storedKey1 == EmptyKey) {
> return null;
> }
> } else {
> if (!acquiredLock) {
> stamp = readLock();
> acquiredLock = true;
> // update local variable
> table = this.table;
> bucket = signSafeMod(keyHash, capacity);
> storedKey1 = table[bucket];
> storedKey2 = table[bucket + 1];
> storedValue1 = table[bucket + 2];
> storedValue2 = table[bucket + 3];
> }
> ```
It can also fix the bug. But is `&& table == this.table` indispensable? If `table != this.table` ,there must be other thread acquire write lock and change `this.table`, which make `validate(stamp)` return false.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lifepuzzlefun commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lifepuzzlefun commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1336159495
@thetumbled i think this fix works, can you add some concurrent test on this 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1308742562
Nice catch! And I think the `ConcurrentLongLongPairHashMap.Section#forEach` method may have the same problem. Could you help to confirm 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1718877376
/pulsarbot 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1700713866
I have update the test code to reproduce the problem. PTAL, thanks. @lordcheng10 @lhotari @eolivelli @BewareMyPower
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1354078103
> There are same problems with `org.apache.pulsar.common.util.collections.ConcurrentLongHashMap`, **which is referenced in many places.**
Yes, the same problem exists here.
Can you fix it？ @thetumbled
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1050528375
##########
pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMapTest.java:
##########
@@ -173,6 +174,62 @@ public void testExpandAndShrink() {
assertEquals(map.capacity(), 8);
}
+ @Test
+ public void testExpandAndShrinkAndGet() throws Throwable {
+ ConcurrentLongLongPairHashMap map = ConcurrentLongLongPairHashMap.newBuilder()
Review Comment:
can we cover with tests also the other classes that you changed please ?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1350309445
I agree with this fix, can you add a test to cover this case？ @thetumbled
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1720392036
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1311505247
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
At this time, `capacity` must be equal to `values.length`.
If we rehash or update the map, the `capacity` will be updated too.
https://github.com/apache/pulsar/blob/0ab1800a3024ed1325d822c4dc6a3267927eca90/pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java#L594
Indeed, it would be more friendly if we use `values.length`.
##########
pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMapTest.java:
##########
@@ -173,6 +174,62 @@ public void testExpandAndShrink() {
assertEquals(map.capacity(), 8);
}
+ @Test
+ public void testExpandAndShrinkAndGet() throws Throwable {
+ ConcurrentLongLongPairHashMap map = ConcurrentLongLongPairHashMap.newBuilder()
Review Comment:
ok.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +322,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / 4);
Review Comment:
For each util class, an item corresponds to multiple array member.
Take `ConcurrentLongLongPairHashMap` for example:
https://github.com/apache/pulsar/blob/0ab1800a3024ed1325d822c4dc6a3267927eca90/pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java#L309
Each item corresponds to 4 array member, that is `table.length==capacity*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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312486152
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashSet.java:
##########
@@ -321,24 +321,20 @@ boolean contains(V value, int keyHash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312486056
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashMap.java:
##########
@@ -354,6 +358,8 @@ V get(K key, int keyHash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ table = this.table;
bucket = signSafeMod(keyHash, capacity);
Review Comment:
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312666391
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +325,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / ITEM_SIZE);
Review Comment:
Btw, it seems that the original code has a bug in calculating the index in `table` for the bucket. I think that it's a a bug in the original code. It seems that there should be a separate `bucketIndex` variable which is calculated by `bucket * ITEM_SIZE`. @merlimat WDYT?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1702500592
Let's first get #21107 reviewed and merged before proceeding with this fix. It's possible that the bugs #21106 and #21108 have been causing ArrayIndexOut0fBoundsExceptions too.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1307180445
There's a checkstyle failure in https://github.com/thetumbled/pulsar/actions/runs/3418835699/jobs/5691621649#step:8:996
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liangyepianzhou commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
liangyepianzhou commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1309742048
Oh my fault, here is a writeLock in the `ConcurrentLongLongPairHashMap.Section#remove`.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1354093618
> > There are same problems with `org.apache.pulsar.common.util.collections.ConcurrentLongHashMap`, **which is referenced in many places.**
>
> Yes, the same problem exists here. Can you fix it？ @thetumbled
i will try to fix 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1321667492
PTAL, thanks. @lhotari @BewareMyPower @eolivelli @WJL3333
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1311487663
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
shouldn't `values.length` be used instead of `this.capacity`?
```suggestion
bucket = signSafeMod(keyHash, values.length);
```
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongPairSet.java:
##########
@@ -331,7 +331,11 @@ private static final class Section extends StampedLock {
boolean contains(long item1, long item2, int hash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(hash, capacity);
+
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 2 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(hash, table.length / 2);
Review Comment:
similar comment here about requesting the rationale for using 2 as the divisior.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -345,7 +348,8 @@ LongPair get(long key1, long key2, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
-
+ // update local variable
+ table = this.table;
bucket = signSafeMod(keyHash, capacity);
Review Comment:
Why is capacity used in this case? (while `table.length / 4` is used initially)
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashSet.java:
##########
@@ -321,24 +321,20 @@ boolean contains(V value, int keyHash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
```suggestion
bucket = signSafeMod(keyHash, values.length);
```
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongPairSet.java:
##########
@@ -353,6 +357,8 @@ boolean contains(long item1, long item2, int hash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ table = this.table;
bucket = signSafeMod(hash, capacity);
Review Comment:
similar comment: why use capacity instead of `table.length / 2`?
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashMap.java:
##########
@@ -332,7 +332,11 @@ private static final class Section<K, V> extends StampedLock {
V get(K key, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+
+ // add local variable here, so OutOfBound won't happen
+ Object[] table = this.table;
+ // calculate table.length / 2 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / 2);
Review Comment:
see previous comment for similar code lines
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +322,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / 4);
Review Comment:
what is the rationale behind choosing 4 as the divisior?
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashMap.java:
##########
@@ -354,6 +358,8 @@ V get(K key, int keyHash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ table = this.table;
bucket = signSafeMod(keyHash, capacity);
Review Comment:
similar comment as before
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312645554
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -361,7 +368,7 @@ LongPair get(long key1, long key2, int keyHash) {
}
}
- bucket = (bucket + 4) & (table.length - 1);
Review Comment:
however, I think that there's a bug. `table.length - 1` is not a power of 2 in all cases.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312485914
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongPairSet.java:
##########
@@ -353,6 +357,8 @@ boolean contains(long item1, long item2, int hash) {
stamp = readLock();
acquiredLock = true;
+ // update local variable
+ table = this.table;
bucket = signSafeMod(hash, capacity);
Review Comment:
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1720956794
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1716823174
> Let's first get #21107 reviewed and merged before proceeding with this fix. It's possible that the bugs #21106 and #21108 have been causing ArrayIndexOut0fBoundsExceptions too.
We are sure that PR https://github.com/apache/pulsar/pull/21107 has nothing to do with ArrayIndexOut0fBoundsExceptions. Could we continue to review this PR? @merlimat @lhotari
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310320076
> > the local variable table will not be updated.
>
> @thetumbled It's not true. `table` variable is a reference not a copy, so its length may change.
In the `rehash` method, the member `this.table` of `Section` will be reallocated a new long[] array, which is different from old table member.
For example:
```
public static void main(String[] args) {
int[] table = new int[]{1, 2};
int[] localTable = table;
System.out.println(localTable.length);
// rehash table to new array.
table = new int[]{3};
System.out.println(localTable.length);
}
```
the result will be following:
<img width="104" alt="image" src="https://user-images.githubusercontent.com/52550727/201110781-e6f6af4e-7196-45db-89c3-0a998f007091.png">
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1336705031
PTAL, thanks. @codelipenghui @hangc0276
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1047957572
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -38,6 +40,7 @@
* <p>Keys <strong>MUST</strong> be >= 0.
*/
public class ConcurrentLongLongPairHashMap {
+ private static final Logger logger = LoggerFactory.getLogger(ConcurrentLongLongPairHashMap.class);
Review Comment:
done.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1354432005
I have fix bugs with `ConcurrentOpenHashMap`, `ConcurrentLongPairSet`, `ConcurrentOpenHashSet`, `ConcurrentLongHashMap`.
PTAL, thanks. @lordcheng10
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1308091219
> There's a checkstyle failure in https://github.com/thetumbled/pulsar/actions/runs/3418835699/jobs/5691621649#step:8:996
done
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1307215295
But I'm still not sure about the following logic
```java
// First try optimistic locking
long storedKey1 = table[bucket];
long storedKey2 = table[bucket + 1];
long storedValue1 = table[bucket + 2];
long storedValue2 = table[bucket + 3];
if (!acquiredLock && validate(stamp)) {
// The values we have read are consistent
if (key1 == storedKey1 && key2 == storedKey2) {
return new LongPair(storedValue1, storedValue2);
```
Even if storedKey1 and storeKey2 matched the expected keys, are storedValue1 and storedValue2 guaranteed to be exactly the associated values? This PR covered the case that table is shrinking, which makes the index invalid. However, could it happen that when retrieving `table[bucket + 2]` and `table[bucket + 3]`, these values have been modified? @lhotari @lordcheng10 @thetumbled
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1309714998
Yes, only `tryOptimisticRead` may cause this issue.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1396346561
The pr had no activity for 30 days, mark with Stale label.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1718900145
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1720429187
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1718723304
/pulsarbot 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312646597
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -361,7 +368,7 @@ LongPair get(long key1, long key2, int keyHash) {
}
}
- bucket = (bucket + 4) & (table.length - 1);
Review Comment:
I'll create a PR to fix the bug since it's not related to this current 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312511699
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +322,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / 4);
Review Comment:
done.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1307161680
This seems to be a bug in changes introduced by #14515 . @lordcheng10 Please review
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1017323333
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,19 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ // read a dirty capacity, cause ArrayIndexOutOfBoundsException
+ logger.error("Read a dirty capacity, fallback to Pessimistic Read. capacity:{}, bucket:{}", capacity, bucket);
Review Comment:
done
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1311339884
> Yes, `table == this.table` seems not necessary. So we can choose one solution from these two approaches.
The second approach is more elegant, and i will test it in pulsar cluster too. The final determination is up to community.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] dragonls commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
dragonls commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310215109
The `ConcurrentLongLongPairHashMap` comes from https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMap.java
Maybe there is the same bug in bookkeeper?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310332760
> The `ConcurrentLongLongPairHashMap` comes from https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongLongHashMap.java
>
> Maybe there is the same bug in bookkeeper, or it only occur in getting stored value?
it seems that there is the same bug in bookkeeper.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310373755
> In the `rehash` method, the member `this.table` of `Section` will be reallocated a new long[] array, which is different from old table member.
>
> ```
> private void rehash(int newCapacity) {
> long[] newTable = new long[4 * newCapacity];
> ...
> table = newTable;
> ...
> }
> ```
This inspires me. Can we fix the `get` issue in the same way?
```java
// add local variable here, so OutOfBound won't happen
long[] table = this.table;
int bucket = signSafeMod(keyHash, capacity);
try {
while (true) {
long storedKey1 = table[bucket];
long storedKey2 = table[bucket + 1];
long storedValue1 = table[bucket + 2];
long storedValue2 = table[bucket + 3];
// compare table to ensure not rehash
if (!acquiredLock && validate(stamp) && table == this.table) {
if (key1 == storedKey1 && key2 == storedKey2) {
return new LongPair(storedValue1, storedValue2);
} else if (storedKey1 == EmptyKey) {
return null;
}
} else {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
// update local variable
table = this.table;
bucket = signSafeMod(keyHash, capacity);
storedKey1 = table[bucket];
storedKey2 = table[bucket + 1];
storedValue1 = table[bucket + 2];
storedValue2 = table[bucket + 3];
}
```
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1019222619
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,24 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
but ArrayIndexOutOfBoundsException maybe caused by a bug or anything else
you cannot reliably say that it is due to a concurrent execution.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1016592154
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,19 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ // read a dirty capacity, cause ArrayIndexOutOfBoundsException
+ logger.error("Read a dirty capacity, fallback to Pessimistic Read. capacity:{}, bucket:{}", capacity, bucket);
+ }
Review Comment:
It's better to check index range instead of catching `ArrayIndexOutOfBoundsException`. Also, it's better not to add a log in this util class.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1016594541
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,19 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ // read a dirty capacity, cause ArrayIndexOutOfBoundsException
+ logger.error("Read a dirty capacity, fallback to Pessimistic Read. capacity:{}, bucket:{}", capacity, bucket);
+ }
Review Comment:
@BewareMyPower I doubt that checking the index range would resolve the issue with optimistic reads in this 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1309899548
> > Nice catch! And I think the `ConcurrentLongLongPairHashMap.Section#forEach` method may have the same problem. Could you help to confirm it?
> > 
i find that, even if there is any thread acquired write lock and trigger shrink/expand process, the local variable `table` will not be updated, so the index used in loop will not exceed the bound.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1719301722
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #18390:
URL: https://github.com/apache/pulsar/pull/18390
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312485724
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, capacity);
Review Comment:
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312630952
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, values.length);
storedKey = keys[bucket];
storedValue = values[bucket];
}
- if (capacity != this.capacity) {
- // There has been a rehashing. We need to restart the search
- bucket = keyHash;
- continue;
- }
-
if (storedKey == key) {
return storedValue != DeletedValue ? storedValue : null;
} else if (storedValue == EmptyValue) {
// Not found
return null;
}
}
-
- ++bucket;
+ bucket = (bucket + 1) & (values.length - 1);
Review Comment:
We iterate the array here. As the length of array is an exponential power of 2, `&(values.length - 1)` is equal to `mod values.length`.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312669549
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +325,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / ITEM_SIZE);
Review Comment:
I'll create a separate PR to fix that issue. The main impact is non-optimal store & lookup since hash collisions will happen when they shouldn't.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312811016
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +325,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / ITEM_SIZE);
Review Comment:
I created #21107 to fix the bugs in the hash bucket algorithm implementation.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1352984542
There are same problems with `org.apache.pulsar.common.util.collections.ConcurrentLongHashMap`, **which is referenced in many places.**
we can reproduce concurrency problem using following code.
```
@Test
public void testExpandAndShrinkAndGet() throws Throwable {
ConcurrentLongHashMap map = ConcurrentLongHashMap.newBuilder()
.expectedItems(2)
.concurrencyLevel(1)
.autoShrink(true)
.mapIdleFactor(0.25f)
.build();
assertEquals(map.capacity(), 4);
ExecutorService executor = Executors.newCachedThreadPool();
// expand hashmap
assertNull(map.put(1, "v1"));
assertNull(map.put(2, "v2"));
assertNull(map.put(3, "v3"));
assertTrue(map.capacity() == 8);
Future<?> future = null;
future = executor.submit(() -> {
map.get(1);
});
// shrink hashmap
assertTrue(map.remove(2, "v2"));
Thread.sleep(1000);
assertTrue(map.remove(3, "v3"));
assertTrue(map.capacity() == 4);
future.get();
executor.shutdown();
}
```
and insert this line of code between array access.
`Thread.sleep(3000);`
<img width="421" alt="image" src="https://user-images.githubusercontent.com/52550727/207858074-b1c44132-dbb7-4f48-957e-c34d3e5b2f2c.png">
The result :
<img width="1365" alt="image" src="https://user-images.githubusercontent.com/52550727/207858203-756c870d-71e6-40e8-8c63-0429400ae07a.png">
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1350238337
> This seems to be a bug in changes introduced by #14515 . @lordcheng10 Please review
@lhotari
Sorry for the late reply, I'll take a look at this question today.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1350300593
> nice catch! I left some comments. @thetumbled
fixed, 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] labuladong commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310301283
> the local variable table will not be updated.
@thetumbled It's not true. `table` variable is a reference, so its length may change.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1019328476
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,24 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
Review Comment:
we can detect whether ArrayIndexOutOfBoundsException is caused by a concurrent execution, if not, we just throw out it.
```
try {
storedKey1 = table[bucket];
storedKey2 = table[bucket + 1];
storedValue1 = table[bucket + 2];
storedValue2 = table[bucket + 3];
} catch (ArrayIndexOutOfBoundsException e) {
// read a dirty capacity, cause ArrayIndexOutOfBoundsException
if (validate(stamp)) {
throw e;
}
if (logger.isDebugEnabled()) {
logger.debug(
"Read a dirty capacity, fallback to Pessimistic Read. capacity:{}, bucket:{}, "
+ "cause:{}",
capacity, bucket, e.getMessage());
}
}
```
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1310531625
So there are two approach, and i have tested try/catch approach in pulsar cluster, which works fine.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1016603961
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -327,10 +330,19 @@ LongPair get(long key1, long key2, int keyHash) {
try {
while (true) {
// First try optimistic locking
- long storedKey1 = table[bucket];
- long storedKey2 = table[bucket + 1];
- long storedValue1 = table[bucket + 2];
- long storedValue2 = table[bucket + 3];
+ long storedKey1 = 0;
+ long storedKey2 = 0;
+ long storedValue1 = 0;
+ long storedValue2 = 0;
+ try {
+ storedKey1 = table[bucket];
+ storedKey2 = table[bucket + 1];
+ storedValue1 = table[bucket + 2];
+ storedValue2 = table[bucket + 3];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ // read a dirty capacity, cause ArrayIndexOutOfBoundsException
+ logger.error("Read a dirty capacity, fallback to Pessimistic Read. capacity:{}, bucket:{}", capacity, bucket);
+ }
Review Comment:
I see. This method could be called in multiple threads, it looks like we can only catch the `ArrayIndexOutOfBoundsException` here.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1720363997
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1311900069
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -322,7 +322,10 @@ private static final class Section extends StampedLock {
LongPair get(long key1, long key2, int keyHash) {
long stamp = tryOptimisticRead();
boolean acquiredLock = false;
- int bucket = signSafeMod(keyHash, capacity);
+ // add local variable here, so OutOfBound won't happen
+ long[] table = this.table;
+ // calculate table.length / 4 as capacity to avoid rehash changing capacity
+ int bucket = signSafeMod(keyHash, table.length / 4);
Review Comment:
Please extract the "magic number" (4) to a constant (static final) field
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312626402
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, values.length);
storedKey = keys[bucket];
storedValue = values[bucket];
}
- if (capacity != this.capacity) {
- // There has been a rehashing. We need to restart the search
- bucket = keyHash;
- continue;
- }
-
if (storedKey == key) {
return storedValue != DeletedValue ? storedValue : null;
} else if (storedValue == EmptyValue) {
// Not found
return null;
}
}
-
- ++bucket;
+ bucket = (bucket + 1) & (values.length - 1);
Review Comment:
just wondering if `signSafeMod` should be used here:
```suggestion
bucket = signSafeMod(bucket + 1, values.length);
```
Please check this also for other locations.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312640340
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -333,25 +334,23 @@ V get(long key, int keyHash) {
if (!acquiredLock) {
stamp = readLock();
acquiredLock = true;
+
+ // update local variable
+ keys = this.keys;
+ values = this.values;
+ bucket = signSafeMod(keyHash, values.length);
storedKey = keys[bucket];
storedValue = values[bucket];
}
- if (capacity != this.capacity) {
- // There has been a rehashing. We need to restart the search
- bucket = keyHash;
- continue;
- }
-
if (storedKey == key) {
return storedValue != DeletedValue ? storedValue : null;
} else if (storedValue == EmptyValue) {
// Not found
return null;
}
}
-
- ++bucket;
+ bucket = (bucket + 1) & (values.length - 1);
Review Comment:
> As the length of array is an exponential power of 2, `&(values.length - 1)` is equal to `mod values.length`.
Oh, that's the trick that is played here...
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1312642482
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -361,7 +368,7 @@ LongPair get(long key1, long key2, int keyHash) {
}
}
- bucket = (bucket + 4) & (table.length - 1);
Review Comment:
This code was explained in another comment, https://github.com/apache/pulsar/pull/18390/files#r1312640340
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1700871898
Great work @thetumbled ! Please check the few minor details that I provided as review comments.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] thetumbled commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
thetumbled commented on PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1352661906
> For the test case, maybe this can help:
>
> https://github.com/apache/bookkeeper/blob/43bdfb35317d9a69840d17df94c5c801a4cbba62/bookkeeper-server/src/test/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashMapTest.java#L424-L489
I have implemented the stress test. But the probability of triggering concurrency problems is pretty low.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on a diff in pull request #18390: [fix][broker] fix bug caused by optimistic locking
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #18390:
URL: https://github.com/apache/pulsar/pull/18390#discussion_r1047944871
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java:
##########
@@ -38,6 +40,7 @@
* <p>Keys <strong>MUST</strong> be >= 0.
*/
public class ConcurrentLongLongPairHashMap {
+ private static final Logger logger = LoggerFactory.getLogger(ConcurrentLongLongPairHashMap.class);
Review Comment:
Remove unused variables.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org