Code review comment for lp:~rogpeppe/gozk/update-event-interface

On 29 September 2011 18:56, Gustavo Niemeyer <email address hidden> wrote:
> Review: Disapprove
>
>
> 2692    -               event := Event{
> 2693    -                       Type:  int(data.event_type),
> 2694    -                       Path:  C.GoString(data.event_path),
> 2695    -                       State: int(data.connection_state),
> 2696    -               }
>
> The key problem with this proposal is the trashing of information coming
> from the server. It's discarding not only why e.g. a state is being
> trashed, but also details like the path which is related to an event.
> This path isn't just being returned from the path that called the watch.
> It's actually part of the wire protocol, and comes from the server side.
>
> Trashing both the session state error information and the paths like that
> is not a good idea.

there is no information in the paths. they are always the same as the path
of the request. if this ever changes, then i'd fully support adding the path,
but as it is Event.Path gives a misleading impression (for instance that Path
might name a child that has been added).

we are trashing no information from the server. if we are then we
should make sure that we panic immediately because the server is doing
something unexpected. [i've just added the requisite check].
the exact specification, that is, what callbacks may be made for a given
API call, is unclear because explanation seems to be omitted in
the ZooKeeper manual itself. i think we owe it to gozk's users to provide
a better defined interface, as you will need to know the details of
the interface
to use the API, and so people will rely on the undocumented behaviour
anyway.

PTAL. the latest version has only two kind of events - node changed events
and session status events. i really think this interface makes sense.

please reconsider.