[raft/scd] Derive implicit subscription ID deterministically - #1656
Conversation
4db755d to
5ff0dfc
Compare
475f91b to
7b88375
Compare
7b88375 to
1e57680
Compare
1e57680 to
293b561
Compare
mickmis
left a comment
There was a problem hiding this comment.
(293b561)
- this UUID is presenting itself as an UUIDv4 but it is actually not: see RFC, this should be random
- I think with the deterministic intent, what should be implemented here would be a v5 (or a v7 with a stretch)
- the OpenAPI does specifically mention the UUID should be a v4 (although I'm not sure how hard of a requirement that is as long as it is a UUID - but that would require to be discussed in the InterUSS weekly)
- I don't think the actualization of the 'now' at each retry is a good idea (in general we should be careful with stuff we put in the context)
The business logic does require us to generate (pseudo-)random data, but we need determinism across nodes. To solve this, have you considered the alternative of (generically) propagating a seed value through the Raft messages? That way all nodes will be able to deterministically generate (pseudo-)random values. I suspect we may encounter this problem again actually?
3be0e79 to
26fd216
Compare
|
@mickmis Thanks for the feedback, I really like the idea of propagating a seed value through the proposal. I just implemented that in a similar way as the timestamp propagation. |
mickmis
left a comment
There was a problem hiding this comment.
LGTM modulo minor comment
| func Generator(ctx context.Context, label string) (*mrand.Rand, error) { | ||
| seed := MustFromContext(ctx) |
There was a problem hiding this comment.
Right now this may panic without the function following the convention MustX, either:
- rename to
MustGenerator(meh) - change signature to pass the seed and not the context
- get seed from context and return error if not present instead of panicking (preferred IMO)
There was a problem hiding this comment.
I think the second option allows us to remain consistent with timestamp and locality. The panic in MustFromContext makes sense in the same way since the seed has to be generated for every single request. I just pushed that.
26fd216 to
7e89936
Compare
7e89936 to
f4c0eee
Compare
Chained PR: #1627 -> #1642 -> #1643 -> #1644 -> #1645 -> #1646 -> #1649 -> #1650 -> #1651 -> #1653 -> #1654 -> #1656 -> #1657 -> #1655 -> #1666 -> #1667 -> #1668 -> #1669
upsertOperationalIntentReference's transaction creates a new random UUID when an implicit subscription is requested. However, this wouldn't execute deterministically across Raft nodes and entry replays.There are two directions to solve this:
parametersmap field to theProposalstruct where each operation can pass its parameters and then passing the parameters in the context as we do for the timestamp and locality to propagate them to business logic. However, this solution is not very elegant and would actually only be used once by this case.