Convert DatacenterMap to CRDT #1225
Conversation
There was a problem hiding this comment.
the biggest change is that the DatacenterMap is now split into two different types for the two different use cases, an XdsDatacenterMap used in relays, and a PhoenixDatacenterMap which is used in proxies
I've reviewed the first two commits, but I want to focus on this piece before doing the rest, because I think should explicitly not have two separate types here. The "relay" and "proxy" distinction, should no longer exist in the codebase, because relays can be proxys and vice versa, and there are services we want to add to watch the proxies themselves that will also use this information and add extra info, so there can't be an individual datacenter map type for each service, they all need to be using the same type.
7e6b90a to
7d9215d
Compare
|
When it's ready to re-review, would you be able to squash the commits or pull out some of the changes so it's easier for me to review? |
|
Sure I'll split it up. |
7d9215d to
bf193f5
Compare
|
Build Succeeded 🥳 Build Id: 4ed305fc-98fa-4cbf-adad-591ac3102522 The following development images have been built, and will exist for the next 30 days: To build this version: |
This is the first pass implementation of using CRDTs for the DatacenterMap.
Other than converting to a CRDT, the biggest change is that the DatacenterMap is now split into two different types for the two different use cases, an XdsDatacenterMap used in relays, and a PhoenixDatacenterMap which is used in proxies, the difference mainly being that the PhoenixDatacenterMap streams the operations to a receiver that is owned by the phoenix task so that it can update its internal map incrementally rather than rely on brute force scanning and a consuming the "secret"
removedlist from the old datacentermap. This is still brute forced in a way since we don't actually consume ops from the Xds stream since it's not capable of that granularity, but means we could add them in the future transparently to the new phoenix code.This is just the first pass, I think the next step would be to encode the CRDT ops into protobuf as a new type, and once it is rolled out remove the old one. This change would allow us to actually take advantage of CRDT, as right now this change does a lot of fakery due to the limitations of how resources are encoded in xDS, and would be a good proof of concept before moving to the much more complicated case of the cluster maps.