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 2021/02/16 11:50:05 UTC
[GitHub] [pulsar] WJL3333 opened a new pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
WJL3333 opened a new pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598
Fixes #9595
### Motivation
make `LocalPolicies` immutable to avoid concurrent modify 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.
### Modifications
make `LocalPolicies` immutable. each time modify field need object copy to avoid change the same instance concurrently.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
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
----------------------------------------------------------------
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] sijie commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-784934611
Ping @merlimat
----------------------------------------------------------------
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] WJL3333 commented on a change in pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
WJL3333 commented on a change in pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#discussion_r586092382
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
##########
@@ -19,37 +19,34 @@
package org.apache.pulsar.common.policies.data;
import static org.apache.pulsar.common.policies.data.Policies.defaultBundle;
-import com.google.common.base.Objects;
+import lombok.EqualsAndHashCode;
+import lombok.ToString;
/**
* Local policies.
*/
+@ToString
+@EqualsAndHashCode
public class LocalPolicies {
Review comment:
yes, you are right. i'll add a test 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-822538472
@WJL3333 You can rebase to the current master branch to check if the CI can 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] WJL3333 commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
WJL3333 commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-823699849
@codelipenghui @315157973 rebase current master. please check again
--
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 #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#discussion_r576944627
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
##########
@@ -52,4 +60,12 @@ public boolean equals(Object obj) {
return false;
}
+ @Override
+ public String toString() {
Review comment:
Also we could similarly take out the `equals()` and `hashCode()` methods.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
##########
@@ -52,4 +60,12 @@ public boolean equals(Object obj) {
return false;
}
+ @Override
+ public String toString() {
Review comment:
Use `@ToString()` annotation on the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-821066752
/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] WJL3333 commented on a change in pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
WJL3333 commented on a change in pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#discussion_r586106361
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
##########
@@ -19,37 +19,34 @@
package org.apache.pulsar.common.policies.data;
import static org.apache.pulsar.common.policies.data.Policies.defaultBundle;
-import com.google.common.base.Objects;
+import lombok.EqualsAndHashCode;
+import lombok.ToString;
/**
* Local policies.
*/
+@ToString
+@EqualsAndHashCode
public class LocalPolicies {
Review comment:
@eolivelli add a test case in `LocalPolicesTest.testMakeLocalPoliciesImmutableSerializationCompatibility`. can you take a look again ?
----------------------------------------------------------------
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] zymap commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-784198718
/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] eolivelli commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-824579343
@merlimat can you take a final look 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] WJL3333 commented on a change in pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
WJL3333 commented on a change in pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#discussion_r577391611
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
##########
@@ -52,4 +60,12 @@ public boolean equals(Object obj) {
return false;
}
+ @Override
+ public String toString() {
Review comment:
thanks for reply. i'll change this.
----------------------------------------------------------------
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] eolivelli merged pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598
--
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] 315157973 commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-822542634
Some points are fixed in this pr https://github.com/apache/pulsar/pull/9900 . Please take a look @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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#discussion_r583443688
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/LocalPolicies.java
##########
@@ -19,37 +19,34 @@
package org.apache.pulsar.common.policies.data;
import static org.apache.pulsar.common.policies.data.Policies.defaultBundle;
-import com.google.common.base.Objects;
+import lombok.EqualsAndHashCode;
+import lombok.ToString;
/**
* Local policies.
*/
+@ToString
+@EqualsAndHashCode
public class LocalPolicies {
Review comment:
can this change impact compatibility with existing clusters ?
Jackson Mapper uses Java reflection
How do we guarantee that the JSON code of this version is compatible with the one of the previous version ?
Can we add tests that try to deserialise a JSON produced with the old version ? or do we already have some ?
----------------------------------------------------------------
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] zymap commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-787911604
Because it's flaky tests fix. I will move this to the next release.
----------------------------------------------------------------
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] eolivelli commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-827396970
This patch cannot be applied to branch-2.7, I am removing the release/2.7.2 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #9598: Make `LocalPolicies` immutable to avoid concurrent modify inconsistent.
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9598:
URL: https://github.com/apache/pulsar/pull/9598#issuecomment-787880686
@WJL3333 Could you please take a look at Enrico's comment?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org