add random number to xDS node ID in google-c2p resolver #26433
Conversation
markdroth
added
the
release notes: no
| // Construct bootstrap JSON. | ||
| Json::Object node = { | ||
| {"id", "C2P"}, | ||
| {"id", absl::StrCat("C2P-", rand())}, |
There was a problem hiding this comment.
Even though we use rand() in a lot of other places in core, it doesn't seem completely correct to use it given:
- AFAIK
rand()isn't generally guaranteed to be thread safe - it's not guaranteed to have been seeded
The backoff code uses it's own random number generator which I believe solves this - this seems like the correct approach to me, any reason not to do what the backoff code is doing instead of using rand() ?
There was a problem hiding this comment.
See discussion in #17891. I think rand() is sufficient for what we need here.
There was a problem hiding this comment.
The broken case I can imagine here binaries which aren't calling srand at the top of main. They would all generate the same random number in their node IDs and so defeat the purpose of the random number.
Another alternative to rand may be to use the absl random library, which takes care of seeding.
That said, since this is used in many other places, this is a wider discussion from this PR so doesn't need to block
There was a problem hiding this comment.
Hmm, actually, I think you're right: having every client report the same random number will kind of defeat the purpose here. Our other uses of rand() inside of C-core are not visible outside of our code, so it doesn't matter as much if there's a little bit of determinism.
Unfortunately, we can't yet use the absl random library, as per https://github.com/grpc/grpc/blob/master/third_party/ABSEIL_MANUAL.md. I've asked @veblush to look into what's blocking us from using it. If we can get that resolved quickly, I can change this code to use that.
There was a problem hiding this comment.
For now, I've changed this to use the C++ random library. PTAL.
2 participants
No description provided.