You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/02/19 23:13:09 UTC
[GitHub] [helix] i3wangyi opened a new pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
i3wangyi opened a new pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(Fixes #784 )
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
The `clone` method is not quite clean in its implementation, readers can hardly tell which `_eventAttributeMap` is the old event versus the new one.
Also, the stack trace in #784 proves the possibility of ConcurrentModificationException. A right and straightforward way are to implement the copy constructor but make it private.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Copy & paste the result of "mvn test")
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on issue #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on issue #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#issuecomment-589860507
The PR is ready to be merged into Master, approved by @dasahcc
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r381845980
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -114,11 +127,6 @@ public String toString() {
}
public ClusterEvent clone(String eventId) {
- ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId);
- newEvent.setCreationTime(_creationTime);
- for (String attributeName : _eventAttributeMap.keySet()) {
Review comment:
@i3wangyi I am a bit confused and not convinced by the root cause: "In ClusterEvent.java, the hashmap update itself while in its for loop". If so, each time this method is called, the `ConcurrentModificationException` would be thrown and it does not work at all.
The maps _eventAttributeMap are different objects. So addAttribute() does not add the attributes to the original event's map, but the new event's different map.
So I think the root cause could be another place (eg. another thread) updating the original event's map while in for loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382803570
##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestClusterEvent.java
##########
@@ -19,16 +19,51 @@
* under the License.
*/
+import java.util.ArrayList;
+import java.util.ConcurrentModificationException;
+import java.util.List;
+
+import org.testng.Assert;
import org.testng.AssertJUnit;
import org.testng.annotations.Test;
-@Test
+
public class TestClusterEvent {
+
@Test
- public void testSimplePutandGet() {
+ public void testSimplePutAndGet() {
ClusterEvent event = new ClusterEvent(ClusterEventType.Unknown);
AssertJUnit.assertEquals(event.getEventType(), ClusterEventType.Unknown);
event.addAttribute("attr1", "value");
AssertJUnit.assertEquals(event.getAttribute("attr1"), "value");
}
+
+ @Test
+ public void testThreadSafeClone() throws InterruptedException {
+ String clusterName = "TestCluster";
+ ClusterEvent event = new ClusterEvent(clusterName, ClusterEventType.Unknown, "testId");
+ for (int i = 0; i < 100; i++) {
+ event.addAttribute(String.valueOf(i), i);
+ }
+ List<Thread> threads = new ArrayList<>();
+ for (int i = 0; i < 10; i++) {
+ threads.add(new Thread(() -> {
+ String threadName = Thread.currentThread().getName();
+ event.addAttribute(threadName, threadName);
Review comment:
This is a reasonable test. Maybe to make it more accurate in this test, we can use a CountDownLatch as a signal right before clone().
```
latch.countDown(); // Tell 10 threads to modify this the attributeMap.
clone();
```
And also add more attributes (say, 20) in each thread just to make the time a bit longer.
This CountDownLatch and more attributes make sure that the attribute adding is not completed yet before clone().
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc merged pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on issue #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm commented on issue #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#issuecomment-588753803
@i3wangyi I am a bit confused and not convinced by the root cause: "In ClusterEvent.java, the hashmap update itself while in its for loop".
The maps `_eventAttributeMap` are different objects. So addAttribute() does not add the attributes to the original event's map, but the new event's different map.
So I think the root cause could be another place (eg. another thread) updating the original event's map while in `for` loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm removed a comment on issue #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm removed a comment on issue #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#issuecomment-588753803
@i3wangyi I am a bit confused and not convinced by the root cause: "In ClusterEvent.java, the hashmap update itself while in its for loop".
The maps `_eventAttributeMap` are different objects. So addAttribute() does not add the attributes to the original event's map, but the new event's different map.
So I think the root cause could be another place (eg. another thread) updating the original event's map while in `for` loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #785: Fix
the ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382767073
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -114,11 +127,6 @@ public String toString() {
}
public ClusterEvent clone(String eventId) {
- ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId);
- newEvent.setCreationTime(_creationTime);
- for (String attributeName : _eventAttributeMap.keySet()) {
Review comment:
@pkuwm Thanks! I was under the impression that copy over a map is a thread-safe operation in some languages(python, e.g). I just tried it in Java, it still suffers the same exception. So I believe the right fix & less code change is to make it ConcurrentHashMap. Also added a test for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #785: Fix
the ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382356368
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -55,6 +55,19 @@ public ClusterEvent(String clusterName, ClusterEventType eventType, String event
_eventId = eventId;
}
+ /**
Review comment:
That's really what I thought initially. But it turns out the variable `_eventAttributeMap` is `final`, which means I cannot mutate the value.
Are you also referring to this
```
ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId); return new ClusterEvent(this, eventId);
newEvent.setCreationTime(_creationTime);
newEvent._eventAttributeMap = new HashMap(__eventAttributeMap);
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #785: Fix
the ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r381822180
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -55,6 +55,19 @@ public ClusterEvent(String clusterName, ClusterEventType eventType, String event
_eventId = eventId;
}
+ /**
Review comment:
Thanks for the PR!
1. Can we just replace clone() logic with what you have in the copy constructor? If the constructor is private, then I think it'd just be simpler to put this "copy" logic in clone() (it seems pretty simple anyways).
2. With that said, do we still need addAttribute() anymore?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #785: Fix
the ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382356368
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -55,6 +55,19 @@ public ClusterEvent(String clusterName, ClusterEventType eventType, String event
_eventId = eventId;
}
+ /**
Review comment:
That's really what I thought initially. But it turns out the variable `_eventAttributeMap` is `final`, which means I cannot mutate the value.
Are you also referring to this
```
ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId); return new ClusterEvent(this, eventId);
newEvent.setCreationTime(_creationTime);
newEvent._eventAttributeMap = new HashMap(__eventAttributeMap);
```
Unless we change the map to `non-final`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r381845980
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -114,11 +127,6 @@ public String toString() {
}
public ClusterEvent clone(String eventId) {
- ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId);
- newEvent.setCreationTime(_creationTime);
- for (String attributeName : _eventAttributeMap.keySet()) {
Review comment:
@i3wangyi I am a bit confused and not convinced by the root cause: "In ClusterEvent.java, the hashmap update itself while in its for loop".
The maps _eventAttributeMap are different objects. So addAttribute() does not add the attributes to the original event's map, but the new event's different map.
So I think the root cause could be another place (eg. another thread) updating the original event's map while in for loop.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #785: Fix
the ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382810480
##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestClusterEvent.java
##########
@@ -19,16 +19,51 @@
* under the License.
*/
+import java.util.ArrayList;
+import java.util.ConcurrentModificationException;
+import java.util.List;
+
+import org.testng.Assert;
import org.testng.AssertJUnit;
import org.testng.annotations.Test;
-@Test
+
public class TestClusterEvent {
+
@Test
- public void testSimplePutandGet() {
+ public void testSimplePutAndGet() {
ClusterEvent event = new ClusterEvent(ClusterEventType.Unknown);
AssertJUnit.assertEquals(event.getEventType(), ClusterEventType.Unknown);
event.addAttribute("attr1", "value");
AssertJUnit.assertEquals(event.getAttribute("attr1"), "value");
}
+
+ @Test
+ public void testThreadSafeClone() throws InterruptedException {
+ String clusterName = "TestCluster";
+ ClusterEvent event = new ClusterEvent(clusterName, ClusterEventType.Unknown, "testId");
+ for (int i = 0; i < 100; i++) {
+ event.addAttribute(String.valueOf(i), i);
+ }
+ List<Thread> threads = new ArrayList<>();
+ for (int i = 0; i < 10; i++) {
+ threads.add(new Thread(() -> {
+ String threadName = Thread.currentThread().getName();
+ event.addAttribute(threadName, threadName);
Review comment:
Actually, one additional thread is good enough if it's adding, say, 100 items into the map. It reduces the cost of thread consumptions by test 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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #785: Fix
the ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382357334
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -114,11 +127,6 @@ public String toString() {
}
public ClusterEvent clone(String eventId) {
- ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId);
- newEvent.setCreationTime(_creationTime);
- for (String attributeName : _eventAttributeMap.keySet()) {
Review comment:
You're right! I'll update the root cause in #785. I was initially unaware of the different objects of `_eventAttributeMap`, cause the old code is readable at first sight and it's not thread-safe.
This fix is still valid under multi-threads. Added one test but making it multi-threading and produce the same issue is non-trivial.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382379284
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -114,11 +127,6 @@ public String toString() {
}
public ClusterEvent clone(String eventId) {
- ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId);
- newEvent.setCreationTime(_creationTime);
- for (String attributeName : _eventAttributeMap.keySet()) {
Review comment:
Can you explain why the copy constructor is different from the original for-loop? Maybe I miss something, but I see that `new HashMap<>(map)` is basically also a for-loop to copy the original map entries, which means, the entries could still change and throws concurrent mod exception for the step `new HashMap<>(map)`.
Please correct me if I am wrong. Appreciate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #785: Fix the
ConcurrentModificationException in ClusterEvent.java
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #785: Fix the ConcurrentModificationException in ClusterEvent.java
URL: https://github.com/apache/helix/pull/785#discussion_r382807147
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/ClusterEvent.java
##########
@@ -114,11 +127,6 @@ public String toString() {
}
public ClusterEvent clone(String eventId) {
- ClusterEvent newEvent = new ClusterEvent(_clusterName, _eventType, eventId);
- newEvent.setCreationTime(_creationTime);
- for (String attributeName : _eventAttributeMap.keySet()) {
Review comment:
Agreed that ConcurrentHashMap could protect 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org