You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "thetumbled (via GitHub)" <gi...@apache.org> on 2023/09/02 01:42:22 UTC
[GitHub] [pulsar] thetumbled opened a new pull request, #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
thetumbled opened a new pull request, #21110:
URL: https://github.com/apache/pulsar/pull/21110
Fixes #21109
### Motivation
There is bug in `forEach` method in util class like `ConcurrentLongLongPairHashMap`.
When it detect there is another thread acquire the write lock, it will fall back to acquire the read lock, which is right. But, it will continue to iterate the array from the last time. As another thread may write the new value before the current index for iterate, it may miss the new value.
So, there are posibilities that it may read the new value or can't read the new value, which may volilate the consistence of data.
### Modifications
Remove the optimistic read logic from `forEach` method in all util class.
```
ConcurrentLongHashMap
ConcurrentLongLongPairHashMap
ConcurrentLongPairSet
ConcurrentOpenHashMap
ConcurrentOpenHashSet
```
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*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
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `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 -->
### 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] BewareMyPower merged pull request #21110: [improve] [doc] Add doc for customized util class
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #21110:
URL: https://github.com/apache/pulsar/pull/21110
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318050310
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
Rather than simply stating "Warning: Do Not Guarantee Thread-Safety," it would be more informative to provide a detailed description of the actual behavior, including what is guaranteed and what is not in terms of thread-safety.
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318059232
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
What about:
Method forEach are designed to be used by only one thread at a time. **When iterate the map with concurrent write, it is possible to see or not to see the new value.** There is no gurantee that if we write value1,value2 and we can see the value2, then we can see value1. We may even encounter two mappings with same key, thus method keys may return a List containg two identical keys.
Bear in mind that the results of aggregate status methods including keys, values, asMap are typically useful only when a map is not undergoing concurrent updates in other threads. Otherwise the results of these methods reflect transient states that may be adequate for monitoring or estimation purposes, **but not for program control**.
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1703638834
PTAL, thanks!
@merlimat @lhotari @eolivelli @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 pull request #21110: [improve] [doc] Add doc for customized util class
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1709675070
> Please modify the PR title. I don't think it should be a "fix". It just adds more explanations in the API doc.
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1707727553
> I agree with @codelipenghui. The reason to implement customized concurrent hash map might be historical and limited to the low Java version (8, or even earlier before Pulsar was open sourced).
>
> ```
> * <li>No boxing/unboxing from long -> Long
> * <li>Open hash map with linear probing, no node allocations to store the values
> ```
>
> These two reasons are not convincing given the benchmark result above. I think we can open a discussion in the mail list to remove these implementations.
Create a discussion here: https://lists.apache.org/thread/482q36czdprlchj26f8sqw0k3bghmcgb.
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 a diff in pull request #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318144918
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
updated.
PTAL, thanks. @codelipenghui
--
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 #21110: [improve] [doc] Add doc for customized util class
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1711401704
/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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1706266534
> I am not sure that adding this constraints to `forEach` helps much.
>
> It is required that if from one thread you modify the map then you see the the effects on the change, in the same thread (or in another thread in case you have some concurrency management utility that guarantees the 'happens before' semantics)
>
> If you are scanning the map in a thread and something else is modifying it (from another thread) then it is legit/expected that you see "some values" of the map, before or after the write.
I will change the implementation as adding a warning in the javadocs, in case of any developers use it incorrectly.
thanks for you 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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318059232
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
What about:
Method forEach are designed to be used by only one thread at a time. When iterate the map with concurrent write, it is possible to see or not to see the new value. There is no gurantee that if we write value1,value2 and we can see the value2, then we can see value1. We may even encounter two mappings with same key, thus method keys may return a List containg two identical keys.
Bear in mind that the results of aggregate status methods including keys, values, asMap are typically useful only when a map is not undergoing concurrent updates in other threads. Otherwise the results of these methods reflect transient states that may be adequate for monitoring or estimation purposes, but not for program control.
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318059232
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
How about:
Method forEach are designed to be used by only one thread at a time. **When iterate the map with concurrent write, it is possible to see or not to see the new value.** There is no gurantee that if we write value1,value2 and we can see the value2, then we can see value1. We may even encounter two mappings with same key, thus method keys may return a List containg two identical keys.
Bear in mind that the results of aggregate status methods including keys, values, asMap are typically useful only when a map is not undergoing concurrent updates in other threads. Otherwise the results of these methods reflect transient states that may be adequate for monitoring or estimation purposes, **but not for program control**.
--
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 #21110: [improve] [doc] Add doc for customized util class
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1711407363
/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 a diff in pull request #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318059232
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
What about:
```
Method forEach are designed to be used by only one thread at a time. When iterate the map with concurrent write, it is possible to see or not to see the new value. There is no gurantee that if we write value1,value2 and we can see the value2, then we can see value1. We may even encounter two mappings with same key, thus method keys may return a List containg two identical keys.
Bear in mind that the results of aggregate status methods including keys, values, asMap are typically useful only when a map is not undergoing concurrent updates in other threads. Otherwise the results of these methods reflect transient states that may be adequate for monitoring or estimation purposes, but not for program control.
```
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1706241761
I am not sure that adding this constraints to `forEach` helps much.
It is required that if from one thread you modify the map then you see the the effects on the change, in the same thread (or in another thread in case you have some concurrency management utility that guarantees the 'happens before' semantics)
If you are scanning the map in a thread and something else is modifying it (from another thread) then it is legit/expected that you see "some values" of the map, before or after the write.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1707612869
I agree with @codelipenghui. The reason to implement customized concurrent hash map might be historical and limited to the low Java version (8, or even more earlier before Pulsar was open sourced).
```
* <li>No boxing/unboxing from long -> Long
* <li>Open hash map with linear probing, no node allocations to store the values
```
These two reasons are not convincing given the benchmark result above. I think we can open a discussion in the mail list to remove these implementations.
--
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 #21110: [improve] [doc] Add doc for customized util class
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1711404389
/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 closed pull request #21110: [improve] [doc] Add doc for customized util class
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled closed pull request #21110: [improve] [doc] Add doc for customized util class
URL: https://github.com/apache/pulsar/pull/21110
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1706707456
Given the benchmark results provided in the [link](https://github.com/apache/pulsar/pull/20647#issuecomment-1607257960), it seems prudent to consider replacing the customized ConcurrentMap with ConcurrentHashMap. The superior performance demonstrated by ConcurrentHashMap in put and remove operations surpasses the benefits derived from garbage collection (GC) optimization.
--
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 #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#issuecomment-1709409318
As the replacement of these customized util class will take a long time, we could add the warning to javadocs first.
Could you help to review? @eolivelli @codelipenghui @BewareMyPower @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 a diff in pull request #21110: [fix] [broker] concurrent write and iterate with ConcurrentLongLongPairHashMap.
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #21110:
URL: https://github.com/apache/pulsar/pull/21110#discussion_r1318144918
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongHashMap.java:
##########
@@ -237,6 +238,12 @@ public void clear() {
}
}
+ /**
+ * Iterate over all the entries in the map and apply the processor function to each of them.
+ * <p>
+ * <b>Warning: Do Not Guarantee Thread-Safety.</b>
+ * @param processor the processor to apply to each entry
+ */
Review Comment:
updated. @codelipenghui
--
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