Skip to content
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

Add TTL to v2 xDS #7868

Closed
mattklein123 opened this issue Aug 8, 2019 · 6 comments · Fixed by #13201
Closed

Add TTL to v2 xDS #7868

mattklein123 opened this issue Aug 8, 2019 · 6 comments · Fixed by #13201
Assignees
Labels
api/v3 Major version release @ end of Q3 2019 area/xds no stalebot Disables stalebot from closing an issue
Milestone

Comments

@mattklein123
Copy link
Member

TTL for discovery resources is being added to the proposed v3 UDPA API. I would like to go ahead and add this into the v2 API as it is something we want at Lyft and is I think generally useful to the community.

cc @envoyproxy/api-shepherds @htuch

@mattklein123 mattklein123 added api/v2 no stalebot Disables stalebot from closing an issue labels Aug 8, 2019
@mattklein123 mattklein123 self-assigned this Aug 8, 2019
@htuch htuch added api/v3 Major version release @ end of Q3 2019 and removed api/v2 labels Aug 29, 2019
@mattklein123
Copy link
Member Author

@wgallagher is going to implement this. He will do a quick summary proposal for review by @htuch @lambdai @fredlas

@fredlas
Copy link
Contributor

fredlas commented Sep 26, 2019

Can we please not do this until at least #7293, and ideally also the draft "delta+state-of-the-world unification" branch mentioned in there, is merged? It has been in the review process for over three months, and the trickle of merge conflicts both small and large (e.g. the addition of scoped_rds_integration_test twice exposing a weird lifetimes bug in the integration test framework, the addition of set_node_on_first_message_only requiring plumbing everywhere) has at this point cost me a solid ~2+ weeks. That maintenance work, needed only because the PR has spent so long stalled waiting for reviews, has gotten unreasonable. It sounds like this would likely cause yet another major chunk.

@mattklein123
Copy link
Member Author

@fredlas fair enough. I have not been tracking that other review. What is needed to finish it?

@fredlas
Copy link
Contributor

fredlas commented Sep 26, 2019

Well, the immediate answer, for just #7293, is "for a senior maintainer to click the button", more or less: it does what it should, and I believe is fully tested. I think the really substantial design discussion already took place in #7108, which was the addition of WatchMap, which is probably the most rewrite-y (as opposed to just extensive refactoring) part of the whole effort.

However, #7293 may or may not be acceptable to merge as is, depending on how averse we are to temporary ugliness. #7293 was my (possibly misguided) attempt to make the review smoother by separating the high level concept of "addition of aggregated delta xDS" from "unification of delta and state-of-the-world code". Where the unified version has the various factories construct polymorphically interchangeable delta/SotW objects, this un-unified one just has these bool is_delta params sprinkled everywhere. It's mostly unobtrusive, but certainly boosts the "files modified" count. (Also, it has my new GrpcMuxImpl awkwardly existing alongside the to-be-replaced one).

I had a draft of the unified version ready when I opened #7293. The plan was that #7293 would get reviewed over the course of a couple of weeks, and then depending on the reviewer's preference, I would either add in the unification draft, or we would merge #7293 and the unification would become a new PR. I left it as a draft until a couple of weeks ago, when I thought just skipping straight to the final product might be another chance to actually get some review attention. That unified version, over at master...fredlas:unified_ADS , is passing almost all of its tests (actually, I think it has passed all of its tests at one time or another, just not all at once so far!). However, it did recently encounter the weird integration test framework lifetimes problem I mentioned in the envoy-dev slack channel.

So: almost all of the new stuff is ready to go immediately, albeit slightly messy. The cleanup of that messiness, and removal of the old code I want to replace, will probably take a few days of work beyond that.

@wgallagher
Copy link

https://docs.google.com/document/d/1-w5Lv9idSbR8RNtGpkj-UCM6at6zslHBnC-VYyhe4DQ/edit#

@htuch @fredlas could you please take a look at this proposal. I'm not very satisfied with it, It's mainly meant as a starting point for discussion.

@htuch
Copy link
Member

htuch commented Oct 7, 2019

Thanks @wgallagher, commented in doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/v3 Major version release @ end of Q3 2019 area/xds no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants