rds: migrate RDS to Subscription model.#1441
Conversation
Legacy REST adapter translates REST JSON API fetches to a v2 Subscription, similar to CDS/SDS/EDS.
| // strong hash instead. | ||
| const std::string manager_identifier = | ||
| config.getString("route_config_name") + MAP_CONCATENATOR + config.getString("cluster"); | ||
| rds.route_config_name() + MAP_CONCATENATOR + rds.SerializeAsString(); |
There was a problem hiding this comment.
Doesn't rds.SerializeAsString() basically include the route config name already? Per your TODO should we just switch to hashing on rds.SerializeAsString() itself?
There was a problem hiding this comment.
Yeah, that was me just mechanically replacing things, thanks for pointing this out. I'm wondering about whether we have concerns if we use std::hash here - it's not crypto strong, the standard doesn't impose any strength requirements on the hash. So far, if we get collisions in xDS update hashes, we can miss updates, in this case we can actually get the wrong route table. Should we use a strong hash from BoringSSL or something? It seems using weak hashes is only legit if you also do a compare against the original strings to ensure there are no collisions, as you would in a simple hash table implementation for example. But, we don't keep these around if we're hashing as we do today.
There was a problem hiding this comment.
In this case, AFAIK unordered_maps don't just hash, they hash and then do a full string compare, so it should be fine. In the API cases, I actually think it's a mistake that we use the hash to decide whether to update or not (though it's nice for performance). Would be good to think about that and see if we want to change that, but that's a separate issue.
There was a problem hiding this comment.
Yep, fine with unordered_map here, just wanted to avoid an additional hash. I've opened #1452 to track this for the general xDS API update checks.
…eady there (duh).
**Description** This commit disables -race flag by default and only explicitly set it on the CI. This improves the local devx since usually the race condition won't be an issue at the initial iteration of changes. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
2 participants
Legacy REST adapter translates REST JSON API fetches to a v2 Subscription, similar to
CDS/SDS/EDS.