[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-04-18 Thread Jaskey
Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 Thank you @zhouxinyu --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-04-18 Thread zhouxinyu
Github user zhouxinyu commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 Merged, thanks @Jaskey , please help close this PR. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-04-01 Thread Jaskey
Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @lizhanhui @zhouxinyu @shroman Any more advice on this pr? and when can this be merged? --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-03-08 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Jaskey I agree that `setServiceState` should be deprecated. I overlooked it was _public_. --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753) Coverage decreased (-0.07%) to 31.449% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753) Coverage decreased (-0.07%) to 31.449% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657) Coverage decreased (-0.02%) to 31.504% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657) Coverage decreased (-0.02%) to 31.504% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657) Coverage decreased (-0.02%) to 31.504% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread Jaskey
Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @shroman Actully setServiceState should be synchroinzed in my opnion, say if I am trying to shutdown, and some other thread B is trying to setServiceState ,which should be waiting for

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread Jaskey
Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @shroman @vongosling Do all guys consider makeing method synchrinized which is locking the consumer object is a better way instead of a more fine-grained lock. If so, I

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-26 Thread lizhanhui
Github user lizhanhui commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 As pointed out, I agree marking shutdown and start `synchronized` to keep code as concise as possible. --- If your project is set up for it, you can reply to this email and have your

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-25 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086) Coverage increased (+0.03%) to 31.548% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-25 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086) Coverage increased (+0.03%) to 31.548% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread shroman
Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 I also think that using `synchronized` fits well here. But I would do it in the following way: 1. Make `serviceState` volatile ```java private volatile ServiceState

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread zhouxinyu
Github user zhouxinyu commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 IMO, consider that `start/shutdown` is not a high frequency usage method, use `synchronized` may be more graceful and readable. please @lizhanhui @shroman help review at your

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078) Coverage increased (+0.1%) to 31.636% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014) Coverage increased (+0.1%) to 31.627% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981) Coverage increased (+0.2%) to 31.72% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981) Coverage increased (+0.2%) to 31.72% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840) Coverage increased (+0.3%) to 31.803% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840) Coverage increased (+0.3%) to 31.803% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread coveralls
Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [![Coverage Status](https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840) Coverage increased (+0.3%) to 31.803% when pulling

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread Ah39
Github user Ah39 commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 **# apache/curator TreeCache Start** public TreeCache start() throws Exception { **Preconditions.checkState(treeState.compareAndSet(TreeState.LATENT,

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread Jaskey
Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Ah39 The problem is not only the synchrinaztion on state's read/write, but on `shutdown` and `start` , `shutdown` and `start` should be syncronized For example, two thread

[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...

2017-02-23 Thread Ah39
Github user Ah39 commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 1.AtomicReference State = new AtomicReference() state.compareAndSet 2.volatile can fix the problem --- If your project is set up for it, you can reply to this email