config: WatchMap: cleaner management of watches#7108
config: WatchMap: cleaner management of watches#7108htuch merged 40 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
|
/assign htuch |
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Thanks, some design level comments to kick this off.
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
|
@fredlas I think it's fine for |
|
What I'm saying is that the opposite is also true: it is also fine for it not to be RAII. That's relevant here because the RAIIified Watch has led to awkward code/design. If that wasn't the case, then sure, the RAIIness could be wherever. However, I definitely perceive the code to now be worse than it was before. |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Can you also ensure full coverage in https://249430-65214191-gh.circle-artifacts.com/0/coverage/coverage.source_common_config_watch_map_impl.cc.html?
Other than this, LGTM and we can ship when comments are resolved.
/wait
|
@htuch Please review WatchMap in #7293 rather than in here. Once you like how it looks over there, I can copy it back here and merge (just to keep that one smaller). I think it has turned out that the context of how I intend to use WatchMap is actually critical for the review. The version of WatchMap in #7293 is much simpler than here: no interface needed, Watch is a simple struct without functions, and the code is overall easier to follow. That simplicity was lost in here due to Watch becoming a real class, with functionality and RAII. The good design patterns those changes were meant as should be taken care of by how the new DeltaSubscriptionImpl in #7293 makes use of Watch/WatchMap. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
@fredlas yeah, the interface in #7293 is fine, I think this is what we agreed to at our last meeting. This is modulo the move to pushing this under |
stale
bot
removed
the
stale
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Interface LGTM, just some tactical implementation comments.
/wait
|
|
||
| void WatchMap::removeWatch(Watch* watch) { | ||
| wildcard_watches_.erase(watch); // may or may not be in there, but we want it gone. | ||
| watches_.erase(watch); |
There was a problem hiding this comment.
Should we assert it's in at least one of these?
There was a problem hiding this comment.
I'd say no; removeWatch is purely for cleanup. Weird as it may be for that to happen, it doesn't mean anything is wrong with the WatchMap.
source/common/config/watch_map.cc
Outdated
| } | ||
|
|
||
| absl::flat_hash_set<Watch*> WatchMap::watchesInterestedIn(const std::string& resource_name) { | ||
| // Note that std::set_union needs sorted sets. Better to do it ourselves with insert(). |
There was a problem hiding this comment.
Could we just use an ordered set? std::less gives a total ordering of pointers in C++14.
There was a problem hiding this comment.
We could, but then updates would be more expensive; watch_interest_ would need to hold ordered sets. Removed that comment; it actually took me a minute to figure out what it was even saying!
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
|
@htuch ready to go? |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Great, thanks! Appreciate the patience in getting the design of this clear.
|
Hooray, thanks very much! |
2 participants
To be used with delta ADS. Could probably be used with the current GrpcMuxImpl. Has the SubscriptionCallbacks interface, so a GrpcMux can just directly pass onConfigUpdate() calls through to the WatchMap, which will then appropriately distribute the various resources to the various watches' SubscriptionCallbacks. #4991
Risk Level: none, not yet built into Envoy
Testing: unit tests for the new class