WIP towards gossiping proto only

[thschroeter]

#629, @tamird, please take a look.

I will need some help with encoding and gossiping PrefixConfigMap as proto.

So far I've added proto.GossipInfoStore and some helpers to copy data to and from gossip.* to proto.GossipInfoStore. If you think this approach is valid, I continue today to define the proto structures for encoding of PrefixConfigMap, and then test gossiping using the new proto structures.

Maye there is a simpler approach, that leads to fewer changes?

I think, it's possible to continue refactoring and use the new proto structures in gossip.infoStore to avoid having to copy data. I still need some time to

• continue refactoring
• make sure gossip still works as expected.

[vivekmenezes]

why do you need to type convert using int()?

[thschroeter]

@tamird do you think it would be better to gossip []byte always as payload in gossip.info?

It seems like gossip is sent on behalf of other packages, so maybe encoding and decoding could happen caller side, e.g. before AddInfo and in the registered callbacks. This could be an alternative to explicitly listing all possible types that we can gossip in gossip.proto.

Anyways, I still have to make changes to crack protofying PrefixConfigMap and to finish this. Will ping you if I get stuck or have the next commit for review.

[thschroeter]

OTOH, it's probably better to centralize decoding and encoding in gossip.

[tamird]

@thschroeter you can basically get the best of both worlds by using gogoproto.Message as payload; encoding and decoding happen in gossip but casting to the correct type happens in the caller.

[thschroeter]

@tamird cool, will do that.

[thschroeter]

Keeping you posted. Got tests passing in config and gossip with config.PrefixConfigMap and gossip.Info as proto. Still need to fix some tests in storage to reach a working solution.

[tamird]

@thschroeter can you push your latest work?

[thschroeter]

@tamird merging my WIP branches into one and will push my changes tomorrow for review. looking forward to your feedback.

[thschroeter]

@tamird PTAL

I will shortly push commits up to PrefixConfigMap and gossip.Info as proto. A test in storage fails (Example_rebalancing). My changes might break gossiping StoreDescriptors. Your ./cockroach start --dev --insecure is also unhappy, so we might get some hints there.

Once this works, it should be relatively easy to complete the PR and send gossip as proto. I'm happy to try a different approach otherwise.

My changes make it cumbersome to gossip basic types, since AddInfo only accepts pointers at the moment. I hope this can be fixed.

I've chosen to write polymorphic fields of proto messages as unions, using gogoproto's onlyone. This way, we can keep the value parameter of gossip.AddInfo an empty interface, matching SetValue gogoproto generates, and the message contents are defined. It would be possible to change to []byte fields when nesting messages, but we would need a handle to the type for unmarshmaling. I like the union approach better.

[thschroeter]

@tamird, does it make sense to keep this PR open as it is, or maybe close it, or break it down into smaller tasks? I've seen configs will be redesigned in #2090. Maybe it would be helpful to focus on the gossip part? Anyways, I'd need some outline of the steps towards completing this.

E.g.

• proto message structure of gossip.Info
• signature of gossip methods, for example gossip.AddInfo(key string, v gogoproto.Message) and gossip.GetInfo(key string, v gogoproto.Message)

I think, gossip groups (tracking minima and maxima of values in the cluster) could require OrderedMessage, that can be unmarshaled and sorted as they are received.

type OrderedMessage interface {
      util.Ordered
      gogoproto.Message
}

[tamird]

@thschroeter yeah, I think focusing on the Gossip bit is the right thing. The end goal is to protofy the return of infoStore.delta(). I think we can get there in the following steps:

• introduce a new type for the delta (it doesn't need to have callbacks, for instance)
• protofy infoMap
• protofy groupMap

Does that help? I'd start with the first bullet.

[thschroeter]

@tamird sounds good, thanks.

[thschroeter]

PTAL

It was easier (for me) to work from the leafs (gossip.Info, config.PrefixConfigMap) towards gossip.InfoStore, to verify intermediate steps.

[tamird]

@thschroeter I'm going through this now; I may rewrite some of the commit history to reduce the back-and-forth on the review; hope you don't mind.

[thschroeter]

@tamird, thanks for taking a look! Please go ahead with rewriting the history.

[tamird]

I've rebased this (as one giant commit) in https://github.com/tamird/cockroach/tree/no-more-gob

I'll continue teasing stuff out tomorrow.

[thschroeter]

@tamird can I close this PR? I think, you're almost done with #629 now, so this is obsolete.

[tamird]

@thschroeter sure, let's keep the discussion in #629 then. Thanks for all your work on this!