-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Xds v3 #139
Xds v3 #139
Conversation
Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>
Github diff is useless. I will review based on diffing directiries:
Why did you add public?
Maybe you could change it in v2 as well?
This was duplicated code right? Now server:
Seems fine. Overall good, you could simplify qualifiers because there are a lot of lines like:
A couple of questions to be answered. Side note: I know how to produce diffs like that but not everyone, please do not dump 5KLOC PR just like that, try to make it easier for other reviewers. |
@slonka thanks for the review! I know it's hard to read this kind of PRs, but i think it's kind of inevitable, go-control-plane had the same huge PRs for implementing v3 envoyproxy/go-control-plane#280 for example, has 3k LOC.
Don't really remember, i will roll this back
We could, but this actually doesn't hurt in v2, but has been deprecated in v3, could update it in v2 in a separate PR
It's not duplicated code, but in v3 the Will simplify qualifiers |
Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>
Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
============================================
+ Coverage 88.55% 88.58% +0.03%
- Complexity 207 412 +205
============================================
Files 24 48 +24
Lines 629 1244 +615
Branches 53 106 +53
============================================
+ Hits 557 1102 +545
- Misses 54 106 +52
- Partials 18 36 +18
Continue to review full report at Codecov.
|
I appreciate all the hard work you put into this. My concern though is that the copy-paste approach will make it very difficult to maintain. I'd rather see a common facade which would implement the important caching logic whenever possible. Separating the core of this project from the particular XDS version should be possible unless it enforces a complete redesign of this project. From a quick skim it seems that it's not completely out of reach. We might need to add some custom improvements to the core logic of the control plane and we'd either duplicate the work (and possibly not be able test v3 in real life before we migrate), or we'd just improve one part (which one?). I'm not completely against the approach you've taken, I see the pragmatism in it, however if this is what needs to be done, I'd love to see a documented strategy/contract for adding improvements with regards to how both versions are treated. |
@chemicL I know that's not the best option, but implementing a version-agnostic option would probably involve a lot more work, I think that generics would not solve the problems we need, we would probably need to wrap some classes into interfaces. I actually didn't intend this to get merged, we just went ahead and did it since we were needing it and it seemed go-control-plane was headed in the same direction. |
@chemicL's comment is spot on regarding the pragmatism but also concern of duplicating critical logic. It does seem like a lot of the new files don't actually import any proto messages, and in some cases they only import them for a doc comment. That would make it seem like we could make a "cache-core" module with shared functionality around stream management, watch status, etc. @sschepens Are you planning any more work on this PR, or should others build on this to try to get to a more minimal and merge-able change (or reach the same conclusion you have that the best path forward is duplication) |
FYI I'll probably take a whack at a PR at the other extreme of reusing as much code as possible at the expense of a bunch of generics which might make the code less understandable. I'm not sure if that will be better or not but then we can compare approaches |
@mpuncel that sounds like a good idea and may pay off in the future api versions too. Are you still planning to submit a PR? If so, when do you suppose to find the time for it? We will need to do it some time later in this quarter or beginning of next, but if you are already working on it, there's no use duplicating the work. |
Following the same implementation pattern than go-control-plane, to support XDS V3 I created a separate cache-v3 and server-v3 packages which are simple copies of the v2 packages but using v3 protos and envoy configured to use v3