Skip to content

route: fast return when route matches failed#17769

Merged
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
soulxu:fast_return_route
Aug 24, 2021
Merged

route: fast return when route matches failed#17769
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
soulxu:fast_return_route

Conversation

@soulxu
Copy link
Member

@soulxu soulxu commented Aug 19, 2021 •
edited
Loading

Signed-off-by: He Jie Xu hejie.xu@intel.com

Commit Message: route: fast return when route matches failed
Additional Description:
Risk Level: low
Testing: unit-test added
Docs Changes: n/a
Release Notes: n/a

@soulxu
route: fast return when match failed
9a8a4e1 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
Copy link
Member Author

soulxu commented Aug 19, 2021

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Aug 19, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17769 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Merge branch 'main' into fast_return_route
b3aa362 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@dmitri-d
Copy link
Contributor

dmitri-d commented Aug 20, 2021

The changes lgtm. Do you think it would be possible to add tests for RouteEntryImplBase::matchRoute() that cover the matching logic? It's a heavy object, but looks like its state is mostly derived from a Route protobuf and Vhost...

soulxu added 2 commits August 23, 2021 07:39
@soulxu
add test
035d401 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
fix format
0c466c6 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
Copy link
Member Author

soulxu commented Aug 23, 2021

The changes lgtm. Do you think it would be possible to add tests for RouteEntryImplBase::matchRoute() that cover the matching logic? It's a heavy object, but looks like its state is mostly derived from a Route protobuf and Vhost...

I added a test. Although I can't assert each check is done or not(since they are class static method), but I test the case when all the conditions are matched, and the case one of them doesn't match.

@mattklein123
Copy link
Member

mattklein123 commented Aug 23, 2021

Thanks this LGTM. Did you verify that all of these new branches have code coverage?

/wait-any

@soulxu
add test
87160da 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
Copy link
Member Author

soulxu commented Aug 24, 2021

Forget test the query_parameter, so added it

@soulxu
Copy link
Member Author

soulxu commented Aug 24, 2021

Thanks this LGTM. Did you verify that all of these new branches have code coverage?

yes, I verified that

Thanks this LGTM. Did you verify that all of these new branches have code coverage?

Do you mean all those fast return branch? the test I added test the case one of them doesn't match. so it should cover all of those fast return branches.

mattklein123
mattklein123 approved these changes Aug 24, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 78b3cdb into envoyproxy:main Aug 24, 2021
mpuncel added a commit to mpuncel/envoy that referenced this pull request Aug 25, 2021
@mpuncel
Merge branch 'main' into mpuncel/sds-hc-sequence-2
d7ac8f1 
* main: (32 commits)
  Stop processing pending H/2 frames if connection transitioned to the closed state
  http2: limit use of deferred resets in the http2 codec to server-side connections
  Abort filter chain iteration on local reply
  Reject or strip fragment from request URI
  ext-authz: merge duplicate headers from client request in check request
  common: introduce stable logger /w examples in DNS  (envoyproxy#17772)
  route: fast return when route matches failed (envoyproxy#17769)
  owners: add owners for dubbo proxy network filter (envoyproxy#17820)
  config/router/tcp_proxy/options: v2 API, boosting and --bootstrap-version CLI removal. (envoyproxy#17724)
  coverage: revert the limit http/cache to 92.6. (envoyproxy#17817)
  network: rename SocketAddressProvider as ConnectionInfoProvider (envoyproxy#17717)
  test: bumping coverage (envoyproxy#17757)
  conn_pool: Minor cleanups to ConnPoolBaseImpl (envoyproxy#17710)
  Split VaryHeader into VaryAllowList and VaryUtils to organize vary-related logic (envoyproxy#17728)
  ext_proc: Make tests more resilient to IPv6 support (envoyproxy#17784)
  Remove invlaid backquote from doc (envoyproxy#17797)
  rocketmq: move to contrib (envoyproxy#17796)
  kafka: upstream kafka facade in mesh-filter (envoyproxy#17783)
  ecds: create shared base class for DynamicFilterConfigProviderImpl (envoyproxy#17735)
  Change log level from debug to trace (envoyproxy#17774)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@soulxu @dmitri-d @mattklein123