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 2020/07/01 08:19:43 UTC
[GitHub] [pulsar] codelipenghui opened a new pull request #7409: Fix batch ackset recycled multiple times.
codelipenghui opened a new pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409
Fixes #7384
### Motivation
Fix batch ackset recycled multiple times. The root cause is a race condition in group ack flush and cumulative Ack. So add recycled state check for the ackset.
### Verifying this change
New unit test added.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409#issuecomment-652902448
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409#discussion_r448357625
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
##########
@@ -1197,11 +1199,13 @@ private BitSetRecyclable(Handle<BitSetRecyclable> recyclerHandle) {
}
public static BitSetRecyclable create() {
- return RECYCLER.get();
+ BitSetRecyclable instance = RECYCLER.get();
+ instance.recycled.set(false);
+ return instance;
}
public void recycle() {
- if (recyclerHandle != null) {
+ if (recyclerHandle != null && recycled.compareAndSet(false, true)) {
Review comment:
Make sense. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409#issuecomment-653963817
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409#discussion_r448806839
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
##########
@@ -1197,11 +1199,13 @@ private BitSetRecyclable(Handle<BitSetRecyclable> recyclerHandle) {
}
public static BitSetRecyclable create() {
- return RECYCLER.get();
+ BitSetRecyclable instance = RECYCLER.get();
+ instance.recycled.set(false);
+ return instance;
}
public void recycle() {
- if (recyclerHandle != null) {
+ if (recyclerHandle != null && recycled.compareAndSet(false, true)) {
Review comment:
@merlimat I have addressed your comments, please take a look again, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409#discussion_r449329131
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
##########
@@ -27,6 +27,7 @@
import java.nio.LongBuffer;
import java.util.Arrays;
import java.util.BitSet;
+import java.util.concurrent.atomic.AtomicBoolean;
Review comment:
```suggestion
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409#discussion_r448328640
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java
##########
@@ -1197,11 +1199,13 @@ private BitSetRecyclable(Handle<BitSetRecyclable> recyclerHandle) {
}
public static BitSetRecyclable create() {
- return RECYCLER.get();
+ BitSetRecyclable instance = RECYCLER.get();
+ instance.recycled.set(false);
+ return instance;
}
public void recycle() {
- if (recyclerHandle != null) {
+ if (recyclerHandle != null && recycled.compareAndSet(false, true)) {
Review comment:
This is still not safe. There cannot be a race condition to resolve the recycling.
In this case it can happen:
1. `thread-1` calls recycle and CAS succeeds
2. `thread-2` gets a the object from pool
3. `thread-3` sees `recycled==false` and does the CAS again, recycle a different object that is in use.
You cannot use Recyclable for objects with a shared ownership. Either change the logic to avoid race conditions, or use ref-counting.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #7409: Fix batch ackset recycled multiple times.
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #7409:
URL: https://github.com/apache/pulsar/pull/7409
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org