Switch to snappy/proto for encoding ring in consul #159
Conversation
jml
changed the title
| <td>{{ $key }}</td> | ||
| <td>{{ $value.State }}</td> | ||
| <td>{{ $value.Hostname }}</td> | ||
| <td>{{ $value.Timestamp | time }}</td> |
There was a problem hiding this comment.
IIRC, this time is from the ingester's clock, but the state is inferred from the distributor's clock.
Either way, this page should either show the distributor's opinion of the current time outside the table (so we have a basis for comparison), or the delta between the current time and the last heartbeat in the table.
tomwilkie
changed the title
tomwilkie
force-pushed
the
protoring
branch
3 times, most recently
from
6bcd711 to
e79df77
Compare
tomwilkie
changed the title
| // InstanceFactory type creates empty instances for use when deserialising | ||
| type InstanceFactory func() interface{} | ||
| // SerDes allows the consult client to serialise and deserialise values. | ||
| type SerDes interface { |
There was a problem hiding this comment.
Codec would be a more traditional name, and would be more consistent with the methods on json & proto.
| } | ||
| return nil | ||
|
|
||
| return d.proto.Decode(bytes) |
There was a problem hiding this comment.
The downside of this method is that it will mask genuine JSON errors. A more precise approach would be to figure out exactly which error is returned when we try to decode protobufs as JSON (https://godoc.org/encoding/json has a bunch), and then type assert on that.
I'm not entirely sure it's worth it, though.
| return &DynamicSerDes{ | ||
| useProto: false, | ||
| json: json, | ||
| proto: proto, |
There was a problem hiding this comment.
A different approach would be to have a vector of SerDes instead of special json and proto ones, and an index of the currently selected format, instead of useProto. That way this would work with future format changes.
Then the public UseProto would become UseVersion and the check for allIngestersCanReadProtos would instead find the minimum supported version.
Not sure it's worth it though. Would help for future compatibility stuff and for tests (because you could pass much simpler codecs & objects).
There was a problem hiding this comment.
Not sure it's worth it though.
This. I intend to delete most of this code once we get into production, and I doubt we'll be doing another move like this in the future, at least to the extent where is worth maintaining this code.
| if d.useProto { | ||
| return d.proto.Encode(msg) | ||
| } | ||
| return d.json.Encode(msg) | ||
| } |
There was a problem hiding this comment.
I'd welcome unit tests for DynamicSerDes.
There was a problem hiding this comment.
Meh, I intend to delete it next week.
| d.mtx.Lock() | ||
| defer d.mtx.Unlock() | ||
| d.useProto = useProto | ||
| if d.useProto != useProto { | ||
| log.Infof("Switching to proto serialization: %v", useProto) |
There was a problem hiding this comment.
Except this isn't true. If useProto is false, then we are switching away from proto encoding.
|
|
||
| if !d.useProto { | ||
| log.Infof("Error decoding json, switching to writing proto: %v", err) | ||
| d.useProto = true |
There was a problem hiding this comment.
@jml perhaps I should do this only if d.proto.Decode succeeds?
| @@ -165,7 +165,7 @@ func (r *IngesterRegistration) heartbeat(tokens []uint32) { | |||
| ingesterDesc.Timestamp = time.Now().Unix() | |||
| ingesterDesc.State = r.state | |||
|
|
|||
| // Set ProtoRing back to true for the case where an existing ingester that didn't understand this field removed it whilst updating the ring. | |||
| // Set ProtoRing back to true for the case where an existing ingestser that didn't understand this field removed it whilst updating the ring. | |||
There was a problem hiding this comment.
Totally not seeing if you're paying attention...
Revert "Merge pull request #159 from weaveworks/protoring"
Fixes #156
(Includes #162 and #161 for now, until they are merged)ring.go:MarshalJSON)Have testing upgrades from master to this locally