Make LocalPolicies immutable to avoid concurrent modify inconsistent.
#9598
Conversation
codelipenghui
requested review from
315157973,
hangc0276,
jiazhai,
merlimat,
sijie,
wolfstudy and
zymap
| @@ -52,4 +60,12 @@ public boolean equals(Object obj) { | |||
| return false; | |||
| } | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
There was a problem hiding this comment.
Use @ToString() annotation on the class
There was a problem hiding this comment.
Also we could similarly take out the equals() and hashCode() methods.
There was a problem hiding this comment.
thanks for reply. i'll change this.
lifepuzzlefun
force-pushed
the
make_LocalPolicies_immutable_to_avoid_concurrent_modification_problem
branch
from
f794b15
to
19c79ee
Compare
|
/pulsarbot run-failure-checks |
|
Ping @merlimat |
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
Show resolved
Hide resolved
|
@WJL3333 Could you please take a look at Enrico's comment? |
|
Because it's flaky tests fix. I will move this to the next release. |
|
/pulsarbot run-failure-checks |
|
@WJL3333 You can rebase to the current master branch to check if the CI can get passed. |
|
Some points are fixed in this pr #9900 . Please take a look @WJL3333 |
… inconsistent. `NamespaceService.updateNamespaceBundles` is not thread safe. when split the same namespace concurrently, both 2 thread can get the same `LocalPolicies` instance from pulsar.getLocalZkCacheService().policiesCache() and `updateNamespaceBundles` change the LocalPolicies.bundles without concurrent protect. which can cause the param `NamespaceBundles` is inconsistent with the serialized data.
default value in default constructor. 2. add timestamp in `ReplicatorTest.testConcurrentReplicator` to avoid namespace create got namespace already exist.
lifepuzzlefun
force-pushed
the
make_LocalPolicies_immutable_to_avoid_concurrent_modification_problem
branch
from
990c2d1
to
2fa1820
Compare
|
@codelipenghui @315157973 rebase current master. please check again |
|
@merlimat can you take a final look please ? |
|
This patch cannot be applied to branch-2.7, I am removing the release/2.7.2 label |
Fixes #9595
Motivation
make
LocalPoliciesimmutable to avoid concurrent modify inconsistent.NamespaceService.updateNamespaceBundlesis not thread safe.when split the same namespace concurrently,
both 2 thread can get the same
LocalPoliciesinstance from pulsar.getLocalZkCacheService().policiesCache()and
updateNamespaceBundleschange the LocalPolicies.bundles withoutconcurrent protect. which can cause the param
NamespaceBundlesis inconsistent with the serialized data.eg.
origin bundle data [1,3,5]
the thread1 split bundle [1,3] and want to set path in zk with bundle data [1, 2, 3,5] and version 3
the thread2 split bundle [3,5] and want to set path in zk with bundle data [1,3, 4, 5] and version 3
one concurrent run can be following:
after thread1 gene bundle data [1,2,3,5] before go to the line zk.setData(version)
thread2 modify the bundle data [1,3,4,5]
then thread1 won the version write. and return success
updateNamespaceBundlesbut the data write is [1,3,4,5]thread2 will retry and try to split [3,5] again.but [3,5] already not occur in bundleData (because modify the same instance concurrently).then the unit test in #9595 fail with bundle not found in namespace.
Modifications
make
LocalPoliciesimmutable. each time modify field need object copy to avoid change the same instance concurrently.Verifying this change
This change is already covered by existing tests, such as V1_AdminApiTest.testNamespaceSplitBundleConcurrent.
Does this pull request potentially affect one of the following parts:
no
Documentation
no