-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
@wgallagher is going to implement this. He will do a quick summary proposal for review by @htuch @lambdai @fredlas |
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. |
@fredlas fair enough. I have not been tracking that other review. What is needed to finish it? |
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 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. |
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. |
Thanks @wgallagher, commented in doc. |
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
The text was updated successfully, but these errors were encountered: