make grpcmux pause/resume come in pair by returning a RAII obj which resumes requests on destruction. by stevenzzzz · Pull Request #11739 · envoyproxy/envoy

stevenzzzz · 2020-06-24T21:28:13Z

make api pause/resume in pair by returning a RAII obj which resumes requests on destruction.

At present, the pause/resume appear in pair in most call sites, besides the clusterManagerImpl::updateClusterCounts(). this PR add some constraint to pause caller that they need to take care of the resume or the paused type-urls auto resume when the returned Cleanup is out of scope.

Ideally caller of Pause should always resume the resource when it's done, this is a step towards that goal.

Commit Message:
Additional Description:
Risk Level: low, mostly refactoring.
Testing: unit tests adjusted
Docs Changes: N/A
Release Notes: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue] #11674

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

…equests on destruction Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only · 2020-06-24T22:54:53Z

PTAL cannot be assigned to this issue.

🐱

Caused by: a #11739 (comment) was created by @stevenzzzz.

see: more, trace.

stevenzzzz · 2020-06-24T22:56:22Z

/assign @htuch

…l be happy Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch

Thanks so much @stevenzzzz for this change, this is a huge improvement to making pause/resume more robust.
/wait

source/common/config/grpc_mux_impl.cc

htuch · 2020-06-25T21:53:14Z

source/common/router/scoped_rds.cc

-  std::unique_ptr<Cleanup> resume_rds;
+  // NOTE: This should be defined after noop_init_manager and resume_rds, as it depends on the
+  // noop_init_manager, and we want a single RDS discovery request to be sent to management server.
+  std::unique_ptr<Cleanup> init_noop_init_manager;


Can you give this a clearer name? It's easy to confuse noop_init_manager and init_noop_init_manager when reading.

how about disposable_init_mgr?

source/common/router/scoped_rds.cc

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2020-06-26T20:44:32Z

thanks

htuch

Looks good, a few minor comments.
/wait

htuch · 2020-06-29T01:15:22Z

source/common/router/scoped_rds.cc

-  // NOTE: This should be defined after noop_init_manager and resume_rds, as it depends on the
-  // noop_init_manager, and we want a single RDS discovery request to be sent to management server.
-  std::unique_ptr<Cleanup> init_noop_init_manager;
+  std::unique_ptr<Init::ManagerImpl> disposable_init_mgr;


Suggestion: call this srds_init_mgr

htuch · 2020-06-29T01:19:24Z

source/common/router/scoped_rds.cc

+  // NOTE: This should be defined after disposable_init_mgr and resume_rds, as it depends on the
+  // disposable_init_mgr, and we want a single RDS discovery request to be sent to management
+  // server.
+  std::unique_ptr<Cleanup> init_disposable_init_mgr;


Suggestion: call this srds_initialization_continuation

done, indeed a better name.

htuch · 2020-06-29T01:21:50Z

test/common/router/scoped_rds_test.cc

 )EOF";
  Protobuf::RepeatedPtrField<ProtobufWkt::Any> resources;
  parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml);
+  init_watcher_.expectReady().Times(1);


Nit: Times(1) is redundant, that is the default.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2020-06-29T16:21:17Z

/retest

repokitteh-read-only · 2020-06-29T16:21:20Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11739 (comment) was created by @stevenzzzz.

see: more, trace.

htuch

LGTM, thanks!

htuch · 2020-06-29T19:20:52Z

@stevenzzzz approved, but master merges are blocked until the security release tomorrow. I'm going to leave #11674 open still until we sort out the story with cluster manager and refcounts (can you comment there? thanks).

stevenzzzz · 2020-06-29T21:11:06Z

sounds good

…

On Mon, Jun 29, 2020 at 3:21 PM htuch ***@***.***> wrote: @stevenzzzz <https://github.com/stevenzzzz> approved, but master merges are blocked until the security release tomorrow. I'm going to leave #11674 <#11674> open still until we sort out the story with cluster manager and refcounts (can you comment there? thanks). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2I6TC3WUHVPGMTDZ2JQSDRZDSSFANCNFSM4OHKXBEQ> .

-- Xin Zhuang

htuch · 2020-07-01T15:20:00Z

@stevenzzzz can you merge master? Thanks.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2020-07-08T02:44:28Z

sorry, had a git problem locally, had to force push.
Synced to upstream head, and addressed harvey's last round review comments.

htuch

@stevenzzzz LGTM, but there is some strange formatting in this PR (hence why I'm sure CI is failing). Can you fix_format?

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2020-07-09T02:13:10Z

seems like a clang version problem, formatted using "tools/code_format/check_format.py fix" with clang10 now.

stevenzzzz · 2020-07-09T14:10:38Z

/retest
(I dont know if it works)

repokitteh-read-only · 2020-07-09T14:10:41Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11739 (comment) was created by @stevenzzzz.

see: more, trace.

…resumes requests on destruction. (envoyproxy#11739) * doc: fix SNI FAQ link (envoyproxy#10227) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * make api pause/resume in pair by returning a RAII obj which resumes requests on destruction Signed-off-by: Xin Zhuang <stevenzzz@google.com> * cancel resume_cds_ on shutdown of clustermanagerImpl Signed-off-by: Xin Zhuang <stevenzzz@google.com> * format errors fix Signed-off-by: Xin Zhuang <stevenzzz@google.com> * move the ABSL_MUST_USE_RESULT attr to the beginning to see if gcc will be happy Signed-off-by: Xin Zhuang <stevenzzz@google.com> * pass by reference type_urls into cleanup in scoped rds Signed-off-by: Xin Zhuang <stevenzzz@google.com> * review fixes per Harvey comments. Signed-off-by: Xin Zhuang <stevenzzz@google.com> * remove unused parameter, weird it wasnt caught previously Signed-off-by: Xin Zhuang <stevenzzz@google.com> * fix-format using clang 10 Signed-off-by: Xin Zhuang <stevenzzz@google.com> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

lizan and others added 9 commits March 2, 2020 14:27

doc: fix SNI FAQ link (envoyproxy#10227)

a9b9f8b

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Merge branch 'master' of https://github.com/envoyproxy/envoy

ff341df

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

963e42e

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

60953f8

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

8991be3

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

9ed6dee

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

make api pause/resume in pair by returning a RAII obj which resumes r…

92dd072

…equests on destruction Signed-off-by: Xin Zhuang <stevenzzz@google.com>

cancel resume_cds_ on shutdown of clustermanagerImpl

79eb46a

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

format errors fix

d0ce022

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot assigned htuch Jun 24, 2020

move the ABSL_MUST_USE_RESULT attr to the beginning to see if gcc wil…

bf897cb

…l be happy Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch suggested changes Jun 25, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 25, 2020

pass by reference type_urls into cleanup in scoped rds

c51ef88

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot removed the waiting label Jun 26, 2020

htuch suggested changes Jun 29, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 29, 2020

stevenzzzz added 3 commits June 28, 2020 22:11

review fixes per Harvey comments.

63476f8

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

3944e77

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' into raii-pause

49d4e1d

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2020

remove unused parameter, weird it wasnt caught previously

1fcc16e

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch previously approved these changes Jun 29, 2020

View reviewed changes

stevenzzzz changed the title ~~[WIP] make grpcmux pause/resume come in pair by returning a RAII obj which resumes requests on destruction.~~ make grpcmux pause/resume come in pair by returning a RAII obj which resumes requests on destruction. Jun 29, 2020

stevenzzzz mentioned this pull request Jun 29, 2020

keep ApiState pause/resume ref-counted, to make sure resume on the state is always called (always in pair). #11674

Closed

stevenzzzz added 3 commits July 6, 2020 09:36

Merge branch 'master' of https://github.com/envoyproxy/envoy

dbbc4a0

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into master

dd8972f

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

merge upstream master

6e0a725

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz dismissed htuch’s stale review via 00817ed July 8, 2020 02:30

stevenzzzz force-pushed the raii-pause branch 2 times, most recently from 00817ed to 6e0a725 Compare July 8, 2020 02:42

stevenzzzz requested a review from dio as a code owner July 8, 2020 02:42

htuch reviewed Jul 8, 2020

View reviewed changes

stevenzzzz added 3 commits July 8, 2020 21:48

fix-format using clang 10

60c22ec

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into master

34408d9

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' into raii-pause

10e67b7

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch merged commit 81b5299 into envoyproxy:master Jul 9, 2020

sschepens mentioned this pull request Jul 13, 2020

ads: SRDS initialization not pausing RDS #11645

Closed

Conversation

stevenzzzz commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 24, 2020

Uh oh!

stevenzzzz commented Jun 24, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stevenzzzz commented Jun 26, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzzzz commented Jun 29, 2020

Uh oh!

repokitteh-read-only bot commented Jun 29, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Jun 29, 2020

Uh oh!

stevenzzzz commented Jun 29, 2020 via email

Uh oh!

htuch commented Jul 1, 2020

Uh oh!

stevenzzzz commented Jul 8, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

stevenzzzz commented Jul 9, 2020

Uh oh!

stevenzzzz commented Jul 9, 2020

Uh oh!

repokitteh-read-only bot commented Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevenzzzz commented Jun 24, 2020 •

edited

Loading