ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-17

zookeeper_init doc needs clarification

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0.0
    • Component/s: c client, documentation
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. ZOOKEEPER-17.patch
      2 kB
      Patrick Hunt
    2. ZOOKEEPER-17.patch
      2 kB
      Patrick Hunt

      Activity

      Hide
      Patrick Hunt added a comment -

      updated the docs to reflect the sf issue detail - add comments for zkinit.

      Show
      Patrick Hunt added a comment - updated the docs to reflect the sf issue detail - add comments for zkinit.
      Hide
      Austin Shoemaker added a comment -

      The documentation states that if the client_id given to zookeeper_init is expired or invalid that a new session will be automatically generated, implying that it will proceed to the CONNECTED state.

      In the implementation an expired or invalid client_id leads to the unrecoverable SESSION_EXPIRED_STATE, which requires closing and reopening a new connection with no client_id specified to continue.

      Since the server has already assigned a replacement client_id it seems logical to follow the header documentation and proceed with the new value, which appears to be possible by removing the if-block that triggers the expired state in check_events (zookeeper.c).

      If the client application needs to know if the session was replaced, it can simply compare the client_id it provided with the client_id upon entering CONNECTED_STATE.

      What do you think?

      Show
      Austin Shoemaker added a comment - The documentation states that if the client_id given to zookeeper_init is expired or invalid that a new session will be automatically generated, implying that it will proceed to the CONNECTED state. In the implementation an expired or invalid client_id leads to the unrecoverable SESSION_EXPIRED_STATE, which requires closing and reopening a new connection with no client_id specified to continue. Since the server has already assigned a replacement client_id it seems logical to follow the header documentation and proceed with the new value, which appears to be possible by removing the if-block that triggers the expired state in check_events (zookeeper.c). If the client application needs to know if the session was replaced, it can simply compare the client_id it provided with the client_id upon entering CONNECTED_STATE. What do you think?
      Hide
      Patrick Hunt added a comment -

      Hrm. I'll withdraw the patch while I look into this further. Thanks for the feedback.

      Show
      Patrick Hunt added a comment - Hrm. I'll withdraw the patch while I look into this further. Thanks for the feedback.
      Hide
      Benjamin Reed added a comment -

      The documentation is incorrect. An invalid or expired session id will result in a zookeeper_handle that is in the unrecoverable SESSION_EXPIRED_STATE. That comment documented old behavior.

      It turns out that processing correctly the situation where the session is expired and a new session is assigned can be tricky. We also wanted the behavior to be consistent with any other case in which an unrecoverable session event is delivered: the zhandle_t becomes invalid.

      We need to fix the doc to say that if the session id is expired or invalid the return zhandle_t will also be invalid.

      Show
      Benjamin Reed added a comment - The documentation is incorrect. An invalid or expired session id will result in a zookeeper_handle that is in the unrecoverable SESSION_EXPIRED_STATE. That comment documented old behavior. It turns out that processing correctly the situation where the session is expired and a new session is assigned can be tricky. We also wanted the behavior to be consistent with any other case in which an unrecoverable session event is delivered: the zhandle_t becomes invalid. We need to fix the doc to say that if the session id is expired or invalid the return zhandle_t will also be invalid.
      Hide
      Patrick Hunt added a comment -

      I updated the patch based on ben's comments, no change to the semantics of the code, just documentation change.

      Does this sufficiently resolve the stated issue?

      Show
      Patrick Hunt added a comment - I updated the patch based on ben's comments, no change to the semantics of the code, just documentation change. Does this sufficiently resolve the stated issue?
      Hide
      Benjamin Reed added a comment -

      +1 Looks great. I'd commit it, but I'm not setup to build the doc...

      Show
      Benjamin Reed added a comment - +1 Looks great. I'd commit it, but I'm not setup to build the doc...
      Hide
      Patrick Hunt added a comment -

      Committed revision 703595.

      Show
      Patrick Hunt added a comment - Committed revision 703595.
      Hide
      Hudson added a comment -

      Integrated in ZooKeeper-trunk #110 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/110/)
      8 FLE Test
      . zookeeper_init doc needs clarification

      Show
      Hudson added a comment - Integrated in ZooKeeper-trunk #110 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/110/ ) 8 FLE Test . zookeeper_init doc needs clarification
      Hide
      Patrick Hunt added a comment -

      3.0.0 has been released, closing issues.

      Show
      Patrick Hunt added a comment - 3.0.0 has been released, closing issues.

        People

        • Assignee:
          Patrick Hunt
          Reporter:
          Patrick Hunt
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development