config: WatchMap: cleaner management of watches #7108

fredlas · 2019-05-30T15:52:38Z

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

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-05-30T15:52:51Z

/assign htuch

source/common/config/watch_map.h

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

Thanks, some design level comments to kick this off.

source/common/config/watch_map.h

Signed-off-by: Fred Douglas <fredlas@google.com>

source/common/config/watch_map.h

Signed-off-by: Fred Douglas <fredlas@google.com>

include/envoy/config/watch_map.h

htuch · 2019-07-16T22:23:15Z

@fredlas I think it's fine for Watch to be RAII as well as the owning DeltaSubscriptionImpl. RAII is a composable pattern.

fredlas · 2019-07-17T19:18:09Z

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

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

fredlas · 2019-07-22T17:50:50Z

@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.

stale · 2019-07-30T03:05:25Z

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!

htuch · 2019-07-30T22:26:43Z

@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 include/envoy; with a pure interface I assume everything under private is now in implementation files rather than the watch_map.h. If you can followup on #7108 (comment) we can merge this. Thanks.

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

Interface LGTM, just some tactical implementation comments.
/wait

source/common/config/watch_map.cc

source/common/config/watch_map.h

source/common/config/watch_map.cc

htuch · 2019-08-06T02:43:56Z

source/common/config/watch_map.cc

+
+void WatchMap::removeWatch(Watch* watch) {
+  wildcard_watches_.erase(watch); // may or may not be in there, but we want it gone.
+  watches_.erase(watch);


Should we assert it's in at least one of these?

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.

htuch · 2019-08-06T02:50:00Z

source/common/config/watch_map.cc

+}
+
+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().


Could we just use an ordered set? std::less gives a total ordering of pointers in C++14.

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!

test/common/config/watch_map_test.cc

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-08-12T19:56:43Z

@htuch ready to go?

htuch

Great, thanks! Appreciate the patience in getting the design of this clear.

fredlas · 2019-08-14T14:27:40Z

Hooray, thanks very much!

fredlas added 9 commits May 29, 2019 13:44

initial WatchMap snapshot

2ff7a0d

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

502e3b7

Signed-off-by: Fred Douglas <fredlas@google.com>

cleanup and comments

3816de1

Signed-off-by: Fred Douglas <fredlas@google.com>

compiles and a basic test passes

2d7a247

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

d823320

Signed-off-by: Fred Douglas <fredlas@google.com>

add two more tests, and fix a bug, thanks unit testing

d30e229

Signed-off-by: Fred Douglas <fredlas@google.com>

test delta too

2ce83f4

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

df14958

Signed-off-by: Fred Douglas <fredlas@google.com>

tiny rearrangement

33aff0f

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot assigned htuch May 30, 2019

fredlas commented May 30, 2019

View reviewed changes

source/common/config/watch_map.h Outdated Show resolved Hide resolved

fredlas added 4 commits May 30, 2019 11:59

spellcheck

67dcc67

Signed-off-by: Fred Douglas <fredlas@google.com>

unmock resourceName to enable switching off of NiceMock

2ac1518

Signed-off-by: Fred Douglas <fredlas@google.com>

add a test, fix another bug, thanks again unit testing

fdb0a18

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

21f428a

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch suggested changes Jun 3, 2019

View reviewed changes

source/common/config/watch_map.h Outdated Show resolved Hide resolved

source/common/config/watch_map.h Outdated Show resolved Hide resolved

source/common/config/watch_map.h Show resolved Hide resolved

htuch added the waiting:any label Jun 3, 2019

repokitteh-read-only bot removed the waiting:any label Jun 4, 2019

fredlas added 6 commits June 6, 2019 15:59

support watches that want to watch everything by providing no names

38f13cc

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

b8b7b08

Signed-off-by: Fred Douglas <fredlas@google.com>

fix compile after merge

eb0f380

Signed-off-by: Fred Douglas <fredlas@google.com>

snapshot

00f7f55

Signed-off-by: Fred Douglas <fredlas@google.com>

change std pair to AddedRemoved

943949a

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

c61a4aa

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas commented Jun 7, 2019

View reviewed changes

source/common/config/watch_map.h Outdated Show resolved Hide resolved

fredlas commented Jun 10, 2019

View reviewed changes

source/common/config/watch_map.h Show resolved Hide resolved

htuch suggested changes Jun 13, 2019

View reviewed changes

source/common/config/watch_map.h Outdated Show resolved Hide resolved

source/common/config/watch_map.h Show resolved Hide resolved

source/common/config/watch_map.h Outdated Show resolved Hide resolved

source/common/config/watch_map.h Show resolved Hide resolved

htuch added the waiting:any label Jun 13, 2019

repokitteh-read-only bot removed the waiting:any label Jun 13, 2019

add another word to the dictionary

8010950

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch reviewed Jul 16, 2019

View reviewed changes

include/envoy/config/watch_map.h Outdated Show resolved Hide resolved

htuch added the waiting:any label Jul 16, 2019

repokitteh-read-only bot removed the waiting:any label Jul 17, 2019

htuch suggested changes Jul 19, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 19, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 30, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 30, 2019

fredlas added 2 commits July 31, 2019 13:03

back to earlier design

8df252b

Signed-off-by: Fred Douglas <fredlas@google.com>

merge conflict, realphabetize BUILD

76bd613

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label Jul 31, 2019

htuch suggested changes Aug 6, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 6, 2019

fredlas added 2 commits August 7, 2019 14:19

clang tidy

4dba42d

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into WAT_watch

692e982

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label Aug 7, 2019

fredlas added 3 commits August 7, 2019 14:52

merge conflict

8a4ae35

Signed-off-by: Fred Douglas <fredlas@google.com>

merge conflict

064fe75

Signed-off-by: Fred Douglas <fredlas@google.com>

snapshot

3a1d2ba

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch approved these changes Aug 13, 2019

View reviewed changes

htuch merged commit 128acb5 into envoyproxy:master Aug 13, 2019

rgs1 mentioned this pull request Aug 14, 2019

Make iterators const #7924

Merged

fredlas mentioned this pull request Sep 26, 2019

Add TTL to v2 xDS #7868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: WatchMap: cleaner management of watches #7108

config: WatchMap: cleaner management of watches #7108

fredlas commented May 30, 2019

fredlas commented May 30, 2019

htuch left a comment

htuch commented Jul 16, 2019

fredlas commented Jul 17, 2019

htuch left a comment

fredlas commented Jul 22, 2019

stale bot commented Jul 30, 2019

htuch commented Jul 30, 2019

htuch left a comment

htuch Aug 6, 2019

fredlas Aug 7, 2019

htuch Aug 6, 2019

fredlas Aug 7, 2019

fredlas commented Aug 12, 2019

htuch left a comment

fredlas commented Aug 14, 2019

config: WatchMap: cleaner management of watches #7108

config: WatchMap: cleaner management of watches #7108

Conversation

fredlas commented May 30, 2019

fredlas commented May 30, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch commented Jul 16, 2019

fredlas commented Jul 17, 2019

htuch left a comment

Choose a reason for hiding this comment

fredlas commented Jul 22, 2019

stale bot commented Jul 30, 2019

htuch commented Jul 30, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch Aug 6, 2019

Choose a reason for hiding this comment

fredlas Aug 7, 2019

Choose a reason for hiding this comment

htuch Aug 6, 2019

Choose a reason for hiding this comment

fredlas Aug 7, 2019

Choose a reason for hiding this comment

fredlas commented Aug 12, 2019

htuch left a comment

Choose a reason for hiding this comment

fredlas commented Aug 14, 2019