You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by "Stu Hood (JIRA)" <ji...@apache.org> on 2012/09/04 21:27:07 UTC
[jira] [Created] (BOOKKEEPER-392) Racey ConcurrentMap usage in java
hedwig-client
Stu Hood created BOOKKEEPER-392:
-----------------------------------
Summary: Racey ConcurrentMap usage in java hedwig-client
Key: BOOKKEEPER-392
URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
Project: Bookkeeper
Issue Type: Bug
Components: hedwig-client
Reporter: Stu Hood
The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (BOOKKEEPER-392) Racey ConcurrentMap usage in
java hedwig-client
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13455095#comment-13455095 ]
Hudson commented on BOOKKEEPER-392:
-----------------------------------
Integrated in bookkeeper-trunk #708 (See [https://builds.apache.org/job/bookkeeper-trunk/708/])
BOOKKEEPER-392: Racey ConcurrentMap usage in java hedwig-client (Stu Hood via sijie) (Revision 1384336)
Result = SUCCESS
sijie :
Files :
* /zookeeper/bookkeeper/trunk/CHANGES.txt
* /zookeeper/bookkeeper/trunk/hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeResponseHandler.java
* /zookeeper/bookkeeper/trunk/hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigClientImpl.java
* /zookeeper/bookkeeper/trunk/hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigPublisher.java
* /zookeeper/bookkeeper/trunk/hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigSubscriber.java
* /zookeeper/bookkeeper/trunk/hedwig-client/src/main/java/org/apache/hedwig/client/netty/ResponseHandler.java
* /zookeeper/bookkeeper/trunk/hedwig-server/src/main/java/org/apache/hedwig/server/common/ByteStringInterner.java
> Racey ConcurrentMap usage in java hedwig-client
> -----------------------------------------------
>
> Key: BOOKKEEPER-392
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-client
> Reporter: Stu Hood
> Assignee: Stu Hood
> Fix For: 4.2.0
>
> Attachments: 392.diff, 392.diff
>
>
> The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (BOOKKEEPER-392) Racey ConcurrentMap usage in java
hedwig-client
Posted by "Stu Hood (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-392?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stu Hood updated BOOKKEEPER-392:
--------------------------------
Attachment: 392.diff
Merges containsKey+get or get+remove calls into gets/removes, respectively. Uses putIfAbsent or precise remove where appropriate. Also replaces the LinkedList in HedwigClientImpl.host2topic with a threadsafe linked list (CLQ).
> Racey ConcurrentMap usage in java hedwig-client
> -----------------------------------------------
>
> Key: BOOKKEEPER-392
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-client
> Reporter: Stu Hood
> Attachments: 392.diff
>
>
> The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (BOOKKEEPER-392) Racey ConcurrentMap usage in
java hedwig-client
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13448270#comment-13448270 ]
Sijie Guo commented on BOOKKEEPER-392:
--------------------------------------
@Stu, the changes looks good.
But I had a question about storeTopic2HostMapping
{code}
+ if (topic2Host.putIfAbsent(pubSubData.topic, host) == null) {
+ if (logger.isDebugEnabled())
+ logger.debug("Stored info for topic: " + pubSubData.topic.toStringUtf8() + ", old host: "
+ + existingHost + ", new host: " + host);
+ }
+ ConcurrentLinkedQueue<ByteString> topicsForHost = host2Topics.get(host);
+ if (topicsForHost == null) {
+ ConcurrentLinkedQueue<ByteString> newTopicsList = new ConcurrentLinkedQueue<ByteString>();
+ topicsForHost = host2Topics.putIfAbsent(host, newTopicsList);
+ if (topicsForHost == null) {
+ topicsForHost = newTopicsList;
+ }
}
+ topicsForHost.add(pubSubData.topic);
}
{code}
if we failed to put host into topic2Host, I don't think we need to put it into host2Topics. And seems that you had assumption that there is not entry for the given host in topic2Host, which is a bit strict.
I had a different change when working for BOOKKEEPER-70.
https://github.com/sijie/bookkeeper/commit/5ac124f2013f4f1d39559c908320dd91184f806c#L6R452
(my changes for clearAllTopicsForHost has a bit problem. It should use conditional remove #remove(key, oldvalue) to remove from concurrent map in my changes)
Could you take a look at it? How is your opinion?
> Racey ConcurrentMap usage in java hedwig-client
> -----------------------------------------------
>
> Key: BOOKKEEPER-392
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-client
> Reporter: Stu Hood
> Attachments: 392.diff
>
>
> The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (BOOKKEEPER-392) Racey ConcurrentMap usage in
java hedwig-client
Posted by "Sijie Guo (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13452721#comment-13452721 ]
Sijie Guo commented on BOOKKEEPER-392:
--------------------------------------
@Stu, thanks for updating the patch.
the changes are for the subtask BOOKKEEPER-364 of BOOKKEEPER-70. We had finished the works for BOOKKEEPER-70 and began to generate the patches for review and commit. But I think it might take a period. since you had a patch for the race condition and the patch looks pretty good, which is almost ready to be in. I would prefer to commit your patch first.
+1 for the new patch.
> Racey ConcurrentMap usage in java hedwig-client
> -----------------------------------------------
>
> Key: BOOKKEEPER-392
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-client
> Reporter: Stu Hood
> Fix For: 4.2.0
>
> Attachments: 392.diff, 392.diff
>
>
> The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (BOOKKEEPER-392) Racey ConcurrentMap usage in java
hedwig-client
Posted by "Stu Hood (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-392?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Stu Hood updated BOOKKEEPER-392:
--------------------------------
Attachment: 392.diff
Good point Sijie: updated.
https://github.com/sijie/bookkeeper/commit/5ac124f2013f4f1d39559c908320dd91184f806c looks like a solid improvement: nice work! It also addresses many of the races I encountered here. What is your timeframe for committing that component of BOOKKEEPER-70? If you think that that linked review will be landing sometime soon (which ticket is it associated with?) I'd be fine with waiting for it to land rather than committing this.
> Racey ConcurrentMap usage in java hedwig-client
> -----------------------------------------------
>
> Key: BOOKKEEPER-392
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-client
> Reporter: Stu Hood
> Fix For: 4.2.0
>
> Attachments: 392.diff, 392.diff
>
>
> The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (BOOKKEEPER-392) Racey ConcurrentMap usage in java
hedwig-client
Posted by "Ivan Kelly (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/BOOKKEEPER-392?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ivan Kelly updated BOOKKEEPER-392:
----------------------------------
Fix Version/s: 4.2.0
> Racey ConcurrentMap usage in java hedwig-client
> -----------------------------------------------
>
> Key: BOOKKEEPER-392
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-392
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-client
> Reporter: Stu Hood
> Fix For: 4.2.0
>
> Attachments: 392.diff
>
>
> The java hedwig-client misuses ConcurrentMap in various ways.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira