feat(clustering) inline version negotiation #8926
Conversation
There was a problem hiding this comment.
code structure seems very good, but needs tests to be sure of corner cases, especially about data transformations and priority logic.
most of my comments are minor, except for some dubious optimizations that reduce readability or cross abstraction responsibilities. especially about merging the accepted and rejected structures, just because they have similar fields.
|
I think maybe after this PR, we should add some test cases to cover wrpc feature. |
Still I need to add another feature first, so we can unblock the global rate limiting. |
StarlightIbuki
force-pushed
the
feat/inline_version_negotiation
branch
from
da4de46
to
b40523e
Compare
There was a problem hiding this comment.
happy to see a much more robust data representation.
there's still an issue, however: while it's perfectly OK to use a map in protobuf, i don't see any advantage here. I've previously mentioned that the amount of data manipulation here makes tests particularly important (because it's hard to be sure of the "shape" of the data produced at every step); now I think that changing replacing those maps with arrays would greatly simplify the code, removing most of the complex transformations.
fix FT-3005
StarlightIbuki
force-pushed
the
feat/inline_version_negotiation
branch
from
d66a69a
to
247b382
Compare
There was a problem hiding this comment.
let's take adding tests as next priority over other wrpc features @suika-kong
Due to refactoring, #8775 has a lot of merging conflicts. So this is a retry of that.
I've also refactored this and designed it a little differently. And I added a check before the config sync process.
fix FT-3005