Re: [PR] Fix/sentinel filter [dubbo-go]
justxuewei merged PR #2621: URL: https://github.com/apache/dubbo-go/pull/2621 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
sonarcloud[bot] commented on PR #2621: URL: https://github.com/apache/dubbo-go/pull/2621#issuecomment-2009573172 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go=2621=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go=2621=false=true)  No data about Coverage  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
r27153733 commented on PR #2621:
URL: https://github.com/apache/dubbo-go/pull/2621#issuecomment-2002755940
> Thanks! A few comments about code style.
>
> In addition, I have another concern: `sentinel.TraceError` is called by
`OnResponse()` previously, but now is called by `Invoke()`. **THE SIDE EFFECT:
You changes the behavior of the filter.**
>
> IMHO, your concern is that the context isn't updated with the latest data,
right? So a possible fix could be:
>
> ```go
> func (fi *FilterInvoker) Invoke(ctx context.Context, invocation
protocol.Invocation) protocol.Result {
>result := fi.filter.Invoke(ctx, fi.next, invocation)
>// do something to copy the context from the latest one
>return fi.filter.OnResponse(ctx, result, fi.invoker, invocation)
> }
> ```
>
> Wdyt?
>
> UPDATE 1: Maybe the above way is not easy to implement. But the main idea
is to share the context between `fi.filter.Invoke()` and
`fi.filter.OnResponse()`.
我也想在fi.filter.Invoke() 和
fi.filter.OnResponse()中共享上下文，很遗憾的是修改FilterInvoker是不够的，必须修改Filter接口的定义。
我曾经想从java中找到灵感，但是...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
sonarcloud[bot] commented on PR #2621: URL: https://github.com/apache/dubbo-go/pull/2621#issuecomment-2002749567 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go=2621=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go=2621=false=true)  No data about Coverage  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
justxuewei commented on code in PR #2621:
URL: https://github.com/apache/dubbo-go/pull/2621#discussion_r1527405272
##
filter/sentinel/filter.go:
##
@@ -110,6 +101,12 @@ type sentinelProviderFilter struct{}
func newSentinelProviderFilter() filter.Filter {
if sentinelProvider == nil {
+ initOnce.Do(func() {
+ err := sentinel.InitDefault()
+ if err != nil {
+ panic(err)
+ }
Review Comment:
```suggestion
if err := sentinel.InitDefault(); err != nil {
panic(err)
}
```
##
filter/sentinel/filter.go:
##
@@ -177,22 +187,27 @@ func (d *sentinelConsumerFilter) Invoke(ctx
context.Context, invoker protocol.In
if b != nil {
// interface blocked
return sentinelDubboConsumerFallback(ctx, invoker, invocation,
b)
+ } else {
+ defer interfaceEntry.Exit()
}
Review Comment:
Ditto.
##
filter/sentinel/filter.go:
##
@@ -129,8 +126,9 @@ func (d *sentinelProviderFilter) Invoke(ctx
context.Context, invoker protocol.In
if b != nil {
// interface blocked
return sentinelDubboProviderFallback(ctx, invoker, invocation,
b)
+ } else {
+ defer interfaceEntry.Exit()
}
Review Comment:
`else` here could be removed.
```suggestion
}
defer interfaceEntry.Exit()
```
##
filter/sentinel/filter.go:
##
@@ -177,22 +187,27 @@ func (d *sentinelConsumerFilter) Invoke(ctx
context.Context, invoker protocol.In
if b != nil {
// interface blocked
return sentinelDubboConsumerFallback(ctx, invoker, invocation,
b)
+ } else {
+ defer interfaceEntry.Exit()
}
- ctx = context.WithValue(ctx, InterfaceEntryKey, interfaceEntry)
methodEntry, b = sentinel.Entry(methodResourceName,
sentinel.WithResourceType(base.ResTypeRPC),
sentinel.WithTrafficType(base.Outbound),
sentinel.WithArgs(invocation.Arguments()...))
if b != nil {
// method blocked
return sentinelDubboConsumerFallback(ctx, invoker, invocation,
b)
+ } else {
+ defer methodEntry.Exit()
}
Review Comment:
Ditto.
##
filter/sentinel/filter.go:
##
@@ -139,13 +137,19 @@ func (d *sentinelProviderFilter) Invoke(ctx
context.Context, invoker protocol.In
if b != nil {
// method blocked
return sentinelDubboProviderFallback(ctx, invoker, invocation,
b)
+ } else {
+ defer methodEntry.Exit()
}
Review Comment:
Ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
sonarcloud[bot] commented on PR #2621: URL: https://github.com/apache/dubbo-go/pull/2621#issuecomment-1997439947 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go=2621=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go=2621=false=true)  No data about Coverage  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
codecov-commenter commented on PR #2621: URL: https://github.com/apache/dubbo-go/pull/2621#issuecomment-1997439878 ## [Codecov](https://app.codecov.io/gh/apache/dubbo-go/pull/2621?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `47.61905%` with `11 lines` in your changes are missing coverage. Please review. > Project coverage is 47.37%. Comparing base [(`b505f89`)](https://app.codecov.io/gh/apache/dubbo-go/commit/b505f89f9da2342c34b04042e39615f5c40ddc5a?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`6665904`)](https://app.codecov.io/gh/apache/dubbo-go/pull/2621?dropdown=coverage=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 9 commits behind head on main. > :exclamation: Current head 6665904 differs from pull request most recent head a8f8936. Consider uploading reports for the commit a8f8936 to get more accurate results | [Files](https://app.codecov.io/gh/apache/dubbo-go/pull/2621?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [filter/sentinel/filter.go](https://app.codecov.io/gh/apache/dubbo-go/pull/2621?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-ZmlsdGVyL3NlbnRpbmVsL2ZpbHRlci5nbw==) | 47.61% | [8 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/dubbo-go/pull/2621?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@Coverage Diff @@ ## main#2621 +/- ## == - Coverage 47.39% 47.37% -0.03% == Files 341 341 Lines 2512225127 +5 == - Hits1190711904 -3 - Misses 1206912076 +7 - Partials 1146 1147 +1 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dubbo-go/pull/2621?dropdown=coverage=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
Re: [PR] Fix/sentinel filter [dubbo-go]
sonarcloud[bot] commented on PR #2621: URL: https://github.com/apache/dubbo-go/pull/2621#issuecomment-1991917099 ## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go=2621=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go=2621=false=true)  No data about Coverage  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go=2621=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go=2621) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
