Change accessor of filterstate to a const shared_ptr ref by gargnupur · Pull Request #9845 · envoyproxy/envoy

gargnupur · 2020-01-27T23:54:39Z

Description: This is part one of the change to add an option to support looking up filterstate from upstream filters. This change just changes the non-const accessor of filterstate to a const shared_ptr ref of non-const filterstate object.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

gargnupur · 2020-01-27T23:56:21Z

/cc @mandarjog

mattklein123

At a high level LGTM but some small comments and also needs tests. Thanks!

/wait

mattklein123 · 2020-01-28T20:45:39Z

include/envoy/stream_info/stream_info.h

Can you just make the reference method above return the shared_ptr? You can return a const reference to the shared_ptr which should make this just as efficient in the common case. Then we don't need this new method.

@mattklein123 : Won't that mean changing a lot more files because we would be changing a commonly used signature?
I agree it would be better going forward...

I doubt there are that many call-sites. I would just fix them.

sure.. updating it then..

define FilterStateSharedPtr (i.e. using FilterStateSharedPtr = std::shared_ptr<FilterState>;) in filter_state.h and use it everywhere possible.

const FilterStateSharedPtr filterState() const override { return filter_state_;} is not correct, as you are returning a const shared_ptr by value. Just leave the const version as it was, or change it to return a shared_ptr to a const object.

also, it's better to return shared_ptr by value as compared to reference, so as to avoid lifecycle issues

We do this in many places to avoid the copy/inc where necessary. Please return by const ref. Thanks!

Thanks for the feedback. PTAL at the PR again!

(I have to say, I learnt about constness with respect to shared_ptr's)

mattklein123 · 2020-01-28T20:45:56Z

include/envoy/stream_info/stream_info.h

Return const ref shared_ptr

mattklein123 · 2020-01-28T20:46:20Z

source/common/stream_info/stream_info_impl.h

not needed here and elsewhere.

mattklein123

Please split this into two changes. The first can just be the shared_ptr conversion with no other upstream changes, and then we can do the upstream change and make sure we have sufficient testing for that. Thank you!

/wait

mattklein123

Generally LGTM with small comments, thank you.

/wait

mattklein123 · 2020-01-31T00:30:40Z

source/common/router/router.cc

In many places in this change you have added a nullptr check where it did not exist before. This pointer should never be null, so you can remove this check here and elsewhere.

I just feel better to have null protection for ptrs :) so that it doesn't bite later on...

removing the extra protection I added

mattklein123 · 2020-01-31T00:30:57Z

source/common/stream_info/stream_info_impl.h

nit: '{}' not needed here and elsewhere.

Signed-off-by: gargnupur <gargnupur@google.com>

…impl.cc Signed-off-by: gargnupur <gargnupur@google.com>

Signed-off-by: gargnupur <gargnupur@google.com>

mattklein123

Thanks!

gargnupur requested review from PiotrSikora, kyessenov and mattklein123 January 27, 2020 23:54

gargnupur force-pushed the nup_filter_state branch from afae9d9 to a78d3d8 Compare January 27, 2020 23:55

mattklein123 self-assigned this Jan 28, 2020

mattklein123 requested changes Jan 28, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 28, 2020

gargnupur requested a review from jplevyak January 29, 2020 21:17

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

gargnupur force-pushed the nup_filter_state branch 3 times, most recently from 4cb26f3 to 66b7d9f Compare January 29, 2020 22:39

mattklein123 requested changes Jan 29, 2020

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jan 29, 2020

gargnupur changed the title ~~Share filterstate between upstream and downstream filters~~ Change accessor of filterstate to a const shared_ptr ref Jan 30, 2020

gargnupur force-pushed the nup_filter_state branch 2 times, most recently from cfca724 to 559cb1c Compare January 30, 2020 00:40

mattklein123 added the waiting label Jan 30, 2020

repokitteh-read-only bot removed the waiting label Jan 30, 2020

gargnupur force-pushed the nup_filter_state branch from 51a4568 to a2e15c9 Compare January 30, 2020 19:44

mattklein123 requested changes Jan 31, 2020

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jan 31, 2020

gargnupur added 5 commits January 31, 2020 10:49

Support looking up filterstate from upstream filter

083fb88

Signed-off-by: gargnupur <gargnupur@google.com>

Fix comments

74e5184

Signed-off-by: gargnupur <gargnupur@google.com>

fix lint

6b71939

Signed-off-by: gargnupur <gargnupur@google.com>

Fixed based on feedback

7d04c64

Signed-off-by: gargnupur <gargnupur@google.com>

Fixed test

401e6d8

Signed-off-by: gargnupur <gargnupur@google.com>

gargnupur added 12 commits January 31, 2020 10:49

Remove addition of upstream filterstate logic

ee733db

Signed-off-by: gargnupur <gargnupur@google.com>

Fix fmt

af1448b

Signed-off-by: gargnupur <gargnupur@google.com>

Fix err in test

357e07b

Signed-off-by: gargnupur <gargnupur@google.com>

Fix err in test

58656c5

Signed-off-by: gargnupur <gargnupur@google.com>

Fix err in test

7d1d2bc

Signed-off-by: gargnupur <gargnupur@google.com>

Fix fmt

dc457d1

Signed-off-by: gargnupur <gargnupur@google.com>

Fix test

cfc1078

Signed-off-by: gargnupur <gargnupur@google.com>

Fix tests

64b9fb9

Signed-off-by: gargnupur <gargnupur@google.com>

Remove null check for shared ptr

ac0bb89

Signed-off-by: gargnupur <gargnupur@google.com>

make it similar as before

c747fe8

Signed-off-by: gargnupur <gargnupur@google.com>

remove fmt from source/extensions/filters/network/common/redis/codec_…

4fcd48c

…impl.cc Signed-off-by: gargnupur <gargnupur@google.com>

remove nullptr check from tcp_proxy_test too

6acd1cf

Signed-off-by: gargnupur <gargnupur@google.com>

gargnupur force-pushed the nup_filter_state branch from 645196a to 6acd1cf Compare January 31, 2020 18:49

mattklein123 approved these changes Jan 31, 2020

View reviewed changes

mattklein123 merged commit 39356ea into envoyproxy:master Jan 31, 2020

gargnupur mentioned this pull request Feb 25, 2020

Add myself to istio/wg-networking-maintainers-data-plane team member istio/community#282

Merged

sunjayBhatia mentioned this pull request Mar 31, 2020

router: inherit upstream connection filter state in upstream request stream info #10458

Merged

Conversation

gargnupur commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gargnupur commented Jan 27, 2020

Uh oh!

mattklein123 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!

gargnupur Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gargnupur Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gargnupur commented Jan 27, 2020 •

edited

Loading

gargnupur Jan 28, 2020 •

edited

Loading

gargnupur Jan 31, 2020 •

edited

Loading