4 ms·

This is, apparently, not correct. Asking for revision 0 causes etcd to stream updates beginning with whatever revision the server has now, plus one, rather than

This is, apparently, not correct. Asking for revision 0 causes etcd to stream updates beginning with whatever revision the server has now, plus one, rather than the first revision. Asking for revision 1 yields all changes. This behavior was not documented. Whoops, looks suspiciously like someone tested the revision integer for truthiness to see if something was passed.

Nullabillity 7y ago
Worse, this is apparently an intentional choice.
lvh 7y ago
Nope. It's because protobufs can't differentiate between nil/unset and a zero value in this field: https://github.com/etcd-io/etcd/blob/53f15caf73b9285d6043009fa64c925d5a8f573c/etcdserver/etcdserverpb/rpc.proto#L684 https://github.com/etcd-io/etcd/blob/53f15caf73b9285d6043009...
- m0zg 7y ago
  Not all protobufs. Protobuf 2 can do it just fine. The decision to not have emptiness support is the dumbest part of Proto 3 IMO.
  - dilap 7y ago
    I don't think it's that crazy a decision, since it was also kind of crazy to pay the cost of "was this field set" for every single field, when the vast majority of the time you're not going to use it. But it can require a bit more more thought in your protocol design. So for example in this case, the options would be - design things such that 0 is ok to mean "most current" (so start revisions at 1) (this is hard after the fact, but if you know from day 0 that missing values for int types will be 0, you can design everything to start at 1) (Edit: maybe this is how etcd works?) - explicitly break out revision into a message type (so you can notice if it's not provided) - use something like "-1" to mean "now", so that 0 isn't overloaded etc... (You could argue maybe the right call for proto3 was to have a flag on a field saying if you want to be able to notice if it was provided or not. Best of all worlds, at cost of a bit of complexity.)
    - m0zg 7y ago
      "Is this field set" costs 1 bit per _optional_ field in Proto2. _Required_ fields (which are also supported by proto2 and not supported by proto3) do not incur this cost. Those flags then would get packed into a bit mask. Not too big a cost, if you ask me. But now you can't figure this out at all without adding another, boolean field, _and setting it separately_, which I'm pretty sure nobody is going to do unless they really have to, leading to the type of issue we're seeing here.
    - wbl 7y ago
      The elegant way to do this is via coproducts and products.
    - gen220 7y ago
      At my place of work, we typically follow the "message type" solution in situations like this. I don't think it's the most legible solution, but it's the best we can do with the proto spec: I always feel like I should qualify these fields with a comment explaining the apparently pointless wrapper. Google themselves provide https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto https://github.com/protocolbuffers/protobuf/blob/master/src/... to deal with this situation. Quoted from the documentation: > Wrappers for primitive (non-message) types. > These types are useful for places where we need to distinguish between the absence of a primitive typed field and its default value. It should probably be advertised more, as we've experienced that default values of optional fields are a surprising feature for smart developers who are new to protobufs. Maybe it's seen as a wart in the design? Getting rid of Null is hard. I kinda wish they went with the approach that all fields are required, unless they are explicitly declared as optional. This is how Rust does it, and people seem to like it.
      - m0zg 7y ago
        I think you got it backwards. The current situation where unset field results in a value is the equivalent of "null", it's the opposite of "getting rid of null". The previous design didn't have that problem.
        7y ago
  - gabetax 7y ago
    You can implement emptiness with the wrapper message types, e.g. StringValue: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto https://github.com/protocolbuffers/protobuf/blob/master/src/... and sometimes defining your own, e.g. nullable lists. It's not elegant, but it is simple.
    - m0zg 7y ago
      But the point is, proto2 had this feature, and it worked without any hacks.

Nullabillity 7y ago

Worse, this is apparently an intentional choice.

lvh 7y ago

Nope. It's because protobufs can't differentiate between nil/unset and a zero value in this field: https://github.com/etcd-io/etcd/blob/53f15caf73b9285d6043009fa64c925d5a8f573c/etcdserver/etcdserverpb/rpc.proto#L684 https://github.com/etcd-io/etcd/blob/53f15caf73b9285d6043009...

m0zg 7y ago
Not all protobufs. Protobuf 2 can do it just fine. The decision to not have emptiness support is the dumbest part of Proto 3 IMO.
- dilap 7y ago
  I don't think it's that crazy a decision, since it was also kind of crazy to pay the cost of "was this field set" for every single field, when the vast majority of the time you're not going to use it. But it can require a bit more more thought in your protocol design. So for example in this case, the options would be - design things such that 0 is ok to mean "most current" (so start revisions at 1) (this is hard after the fact, but if you know from day 0 that missing values for int types will be 0, you can design everything to start at 1) (Edit: maybe this is how etcd works?) - explicitly break out revision into a message type (so you can notice if it's not provided) - use something like "-1" to mean "now", so that 0 isn't overloaded etc... (You could argue maybe the right call for proto3 was to have a flag on a field saying if you want to be able to notice if it was provided or not. Best of all worlds, at cost of a bit of complexity.)
  - m0zg 7y ago
    "Is this field set" costs 1 bit per _optional_ field in Proto2. _Required_ fields (which are also supported by proto2 and not supported by proto3) do not incur this cost. Those flags then would get packed into a bit mask. Not too big a cost, if you ask me. But now you can't figure this out at all without adding another, boolean field, _and setting it separately_, which I'm pretty sure nobody is going to do unless they really have to, leading to the type of issue we're seeing here.
  - wbl 7y ago
    The elegant way to do this is via coproducts and products.
  - gen220 7y ago
    At my place of work, we typically follow the "message type" solution in situations like this. I don't think it's the most legible solution, but it's the best we can do with the proto spec: I always feel like I should qualify these fields with a comment explaining the apparently pointless wrapper. Google themselves provide https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto https://github.com/protocolbuffers/protobuf/blob/master/src/... to deal with this situation. Quoted from the documentation: > Wrappers for primitive (non-message) types. > These types are useful for places where we need to distinguish between the absence of a primitive typed field and its default value. It should probably be advertised more, as we've experienced that default values of optional fields are a surprising feature for smart developers who are new to protobufs. Maybe it's seen as a wart in the design? Getting rid of Null is hard. I kinda wish they went with the approach that all fields are required, unless they are explicitly declared as optional. This is how Rust does it, and people seem to like it.
    - m0zg 7y ago
      I think you got it backwards. The current situation where unset field results in a value is the equivalent of "null", it's the opposite of "getting rid of null". The previous design didn't have that problem.
      - 7y ago
- gabetax 7y ago
  You can implement emptiness with the wrapper message types, e.g. StringValue: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto https://github.com/protocolbuffers/protobuf/blob/master/src/... and sometimes defining your own, e.g. nullable lists. It's not elegant, but it is simple.
  - m0zg 7y ago
    But the point is, proto2 had this feature, and it worked without any hacks.