contrib-sipproxy: rework sipproxy stats#24165
Merged
mattklein123 merged 12 commits intoenvoyproxy:mainfrom Dec 2, 2022
Merged
Conversation
Signed-off-by: Felix Du <durd07@gmail.com>
Contributor
|
CI needs to be sorted out. Thanks! /retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Felix Du <felix.du@nokia-sbell.com>
Signed-off-by: Felix Du <durd07@gmail.com>
Signed-off-by: Felix Du <durd07@gmail.com>
Member
Author
|
@dorisd0102 Please help to review this PR. |
Contributor
|
LGTM |
mattklein123
requested changes
Nov 28, 2022
Comment on lines
+36
to
+42
| try { | ||
| if (cache_manager_.at(type, key) != val) { | ||
| should_update_tra = true; | ||
| } | ||
| } catch (std::out_of_range) { | ||
| should_update_tra = true; | ||
| } |
Member
There was a problem hiding this comment.
Please avoid using exceptions for basic control flow. Just use find(). Same elsewhere.
Member
Author
There was a problem hiding this comment.
Hi @mattklein123 ,
Thanks for your comments, but why exceptions are not recommended?
I want to implement a similiar method like std::map::at
Member
There was a problem hiding this comment.
Because a) we are trying to avoid exceptions at this point and b) this is not a good use of exceptions. Use find to return an iterator.
Comment on lines
+262
to
+264
| /** | ||
| * NOTE: This may throw exception std::out_of_range if type or key doesn't exist. | ||
| */ |
Member
There was a problem hiding this comment.
Same comment. Don't use exceptions for this.
Signed-off-by: Felix Du <felix.du@nokia-sbell.com>
mattklein123
requested changes
Nov 30, 2022
Comment on lines
+35
to
+37
| bool should_update_tra = true; | ||
| if (cache_manager_.contains(type, key) && (cache_manager_.at(type, key) == val)) { | ||
| should_update_tra = false; |
Member
There was a problem hiding this comment.
You can void a double lookup by using find(). Same elsewhere.
Comment on lines
263
to
265
| * NOTE: This may throw exception std::out_of_range if type or key doesn't exist. | ||
| * use contains(type, key) to check exist first. | ||
| */ |
Member
There was a problem hiding this comment.
Rework to always use find. Never us at.
Member
Author
There was a problem hiding this comment.
@mattklein123 Thanks for your comments.
Firstly I want to implement a find method like what does as std::map, but I can't figure out the solution because different iterator maybe returned in a nested map, so write a method called at which return empty value if not exist.
[[ret_type]] find(const T& type, const K& key) {
auto it = caches_.find(type);
if (it != caches_.cend()) {
return it->second.find(key); // iterator of Cache<K, V>::iterator
} else {
return [[ the iterator of caches_ ]]
}
}
Contributor
There was a problem hiding this comment.
drive-by comment: if your goal is to return an optional value if it's in one of several collections, you could return absl::optional, or OptRef (see envoy/common/optref.h) if you want V to be a reference.
Member
Author
There was a problem hiding this comment.
Thanks @jmarantz , I already aware of this, but I don't want to make thing more complex.
Member
There was a problem hiding this comment.
Yes this is what you should do, so let's please do that.
/wait
Signed-off-by: Felix Du <felix.du@nokia-sbell.com>
Signed-off-by: Felix Du <felix.du@nokia-sbell.com>
added 3 commits
December 2, 2022 02:42
Member
Author
|
@mattklein123 @jmarantz |
jpsim
added a commit
to alyssawilk/envoy
that referenced
this pull request
Dec 2, 2022
* origin/main: Start slow start window on first successful HC, make slow start re-enterable (envoyproxy#23946) contrib-sipproxy: rework sipproxy stats (envoyproxy#24165) build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (envoyproxy#24292) build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (envoyproxy#24274) build(deps): bump actions/upload-artifact from 2 to 3 (envoyproxy#24121) build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (envoyproxy#24273) build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (envoyproxy#24272) Allow mobile/library/common/jni/ to be built on non-Android platforms. (envoyproxy#24299) generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (envoyproxy#24220) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (envoyproxy#24243) Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (envoyproxy#24182) Fix mobile/tools/check_format.sh to re-apply envoyproxy#2698 (envoyproxy#24300) upstream: don't require `source_address` in `upstream_bind_config` (envoyproxy#24250) Quiche roll 20221201150327 (envoyproxy#24290) Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
added a commit
that referenced
this pull request
Dec 2, 2022
…/docs/sphinx-5.3.0 * origin/main: (50 commits) ci: Bump mobile build images (#24317) ci: update llvm on bazel CI (#24296) ci: Fix flaky coverage limit (#24320) ci: Fix change detection (part 2) (#24264) Start slow start window on first successful HC, make slow start re-enterable (#23946) contrib-sipproxy: rework sipproxy stats (#24165) build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (#24292) build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (#24274) build(deps): bump actions/upload-artifact from 2 to 3 (#24121) build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (#24273) build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (#24272) Allow mobile/library/common/jni/ to be built on non-Android platforms. (#24299) generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (#24220) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (#24243) Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (#24182) Fix mobile/tools/check_format.sh to re-apply #2698 (#24300) upstream: don't require `source_address` in `upstream_bind_config` (#24250) Quiche roll 20221201150327 (#24290) Cleanup threading/ownership semantics of PlatformBridgeCertValidator (#2713) Add unit tests of the PlatformBridgeCertValidator (#2704) ... Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
added a commit
that referenced
this pull request
Dec 2, 2022
…/docs/pygments-2.13.0 * origin/main: (25 commits) ci: Bump mobile build images (#24317) ci: update llvm on bazel CI (#24296) ci: Fix flaky coverage limit (#24320) ci: Fix change detection (part 2) (#24264) Start slow start window on first successful HC, make slow start re-enterable (#23946) contrib-sipproxy: rework sipproxy stats (#24165) build(deps): bump github/codeql-action from 2.1.32 to 2.1.35 (#24292) build(deps): bump node from `c59fb39` to `80844b6` in /examples/ext_authz/auth/http-service (#24274) build(deps): bump actions/upload-artifact from 2 to 3 (#24121) build(deps): bump mysql from `96439dd` to `66efaaa` in /examples/mysql (#24273) build(deps): bump grpcio from 1.50.0 to 1.51.1 in /examples/grpc-bridge/client (#24272) Allow mobile/library/common/jni/ to be built on non-Android platforms. (#24299) generic proxy: added drain support to generic proxy to doing graceful closes on connections when possible (#24220) Reduce Route/VirtualHost memory utilization by avoiding RateLimitPolicy instances when not needed (#24243) Reduce VirtualHost memory utilization by avoiding CatchAllVirtualCluster when not needed (#24182) Fix mobile/tools/check_format.sh to re-apply #2698 (#24300) upstream: don't require `source_address` in `upstream_bind_config` (#24250) Quiche roll 20221201150327 (#24290) Cleanup threading/ownership semantics of PlatformBridgeCertValidator (#2713) Add unit tests of the PlatformBridgeCertValidator (#2704) ... Signed-off-by: JP Simard <jp@jpsim.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Felix Du durd07@gmail.com
Commit Message: rework sipproxy stats to improve performance
Additional Description:
Risk Level: Low
Testing: Ut/Mt
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]