You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/02/21 14:00:48 UTC

[GitHub] [zookeeper] swallez opened a new pull request #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

swallez opened a new pull request #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value
URL: https://github.com/apache/zookeeper/pull/1257
 
 
   When packets are added to ClientCnxn's outgoing packet queue we ensure there's no conflict with an ongoing flush of that queue because of connection loss.
   
   Synchronization used to be on the state field's value. This value is both not stable (its value changes over time), possibly causing improper synchronization, and global, which can cause contention in applications that run several ZooKeeper clients.
   
   We now synchronize on outgoingQueue which is both local to a ClientCnxn's instance and stable.

----------------------------------------------------------------
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

[GitHub] [zookeeper] swallez commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

Posted by GitBox <gi...@apache.org>.
swallez commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value
URL: https://github.com/apache/zookeeper/pull/1257#issuecomment-595368157
 
 
   Thanks for the review @maoling!
   
   Synchronizing on `outgoingQueue` or a separate `objectLock` has the exact same performance since no other synchronization statement exists for `outgoingQueue`, including in its implementation (I checked the code for `LinkedBlockingDeque`).
   
   In terms of future extensibility I don't think this is a concern as `outgoingQueue` is used in very few places, which are where synchronization is needed.
   
   And yes, `enum` is great to represent enumerated values but not so great for thread synchronization 😉 

----------------------------------------------------------------
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

[GitHub] [zookeeper] maoling commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

Posted by GitBox <gi...@apache.org>.
maoling commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value
URL: https://github.com/apache/zookeeper/pull/1257#issuecomment-590867118
 
 
   @swallez 
   - `[ERROR] Synchronization performed on java.util.concurrent.LinkedBlockingDeque in org.apache.zookeeper.ClientCnxn.queuePacket(RequestHeader, ReplyHeader, Record, Record, AsyncCallback, String, String, Object, ZooKeeper$WatchRegistration, WatchDeregistration) [org.apache.zookeeper.ClientCnxn] At ClientCnxn.java:[line 1645] `
   
   - JLM_JSR166_UTILCONCURRENT_MONITORENTER: This method performs synchronization an object that is an instance of a class from the java.util.concurrent package  (or its subclasses). Instances of these classes have their own concurrency  control mechanisms that are orthogonal to the synchronization provided by the  Java keyword synchronized. For example, synchronizing on an AtomicBoolean  will not prevent other threads from modifying the AtomicBoolean. Such code may be correct, but should be  carefully reviewed and documented, and may confuse people who have to  maintain the code at a later date.

----------------------------------------------------------------
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

[GitHub] [zookeeper] maoling commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

Posted by GitBox <gi...@apache.org>.
maoling commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value
URL: https://github.com/apache/zookeeper/pull/1257#issuecomment-593204387
 
 
   looks good. I still have another thought(no binding for the following, you can ignore it):
   - Since the root cause is a narrow windows between `queuePacket` and `cleanup`, so synchronized `objectLock` is also an alternative way? which one is better? Since `outgoingQueue` is a critical Queue for client to talk with server, synchronized `outgoingQueue` will have performance and future program extensibility issue?
   
   Haha, I also test for the global and inner wording by the following way:
   - new two different zookeeper clients, create some znodes, printing the hashcode of `outgoingQueue` and `state`. They really hold different `outgoingQueue`, but the same `state`. 
   - synchronized `Enum` is also not thread-safe
   - In a word, `Enum` is a heresy:)

----------------------------------------------------------------
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

[GitHub] [zookeeper] swallez commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

Posted by GitBox <gi...@apache.org>.
swallez commented on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value
URL: https://github.com/apache/zookeeper/pull/1257#issuecomment-592004575
 
 
   @maoling I added a commit to instruct SpotBugs that we know what we're doing, following the approach used to allow `synchronized (waitingEvents)` in that same 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

[GitHub] [zookeeper] maoling edited a comment on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

Posted by GitBox <gi...@apache.org>.
maoling edited a comment on issue #1257: ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value
URL: https://github.com/apache/zookeeper/pull/1257#issuecomment-593204387
 
 
   looks good. I still have another thought(no binding for the following, you can ignore it):
   - Since the root cause is a narrow windows between `queuePacket` and `cleanup`, so synchronized `objectLock` is also an alternative way? which one is better? Since `outgoingQueue` is a critical Queue for client to talk with server, synchronized `outgoingQueue` will have performance and future program extensibility issue?
   
   Haha, I also test for the `global` and `inner` wording by the following way:
   - new two different zookeeper clients, create some znodes, printing the `hashcode` of `outgoingQueue` and `state`. They really hold different `outgoingQueue`, but the same hashcode of `state`. it really confuses me.
   
   - I believe different clients will have different `state` instance, otherwise when one client calls `close()`(set `state` to `CLOSE`), it will affect another client. However, using following ways cannot reason about it.
   ```
   System.out.println(zk1.getState() == zk2.getState()); //true
   System.out.println(zk1.getState().equals(zk2.getState())); //true
   System.out.println(zk1.getState().hashCode() == zk2.getState().hashCode()); //true
   System.out.println(System.identityHashCode(zk1.getState()) == System.identityHashCode(zk2.getState().hashCode()); //true
   ```
   
   `javap` to see the bytecode, the value set to `state` is `public static final`, so it's really global-shared by multi-clients.
   ```
   public final class org.apache.zookeeper.States extends java.lang.Enum<org.apache.zookeeper.States> {
     public static final org.apache.zookeeper.States CONNECTING;
     public static final org.apache.zookeeper.States NOT_CONNECTED;
     ********************************************
     private static final org.apache.zookeeper.States[] $VALUES;
     public static org.apache.zookeeper.States[] values();
   ```
   
   - synchronized `Enum` is also not thread-safe. Look at my demo attached in JIRA.
   - In a word, `Enum` is a heresy:)

----------------------------------------------------------------
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