Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ftr: Grpc Protocol Support #311

Merged
merged 24 commits into from
Jan 17, 2020
Merged

ftr: Grpc Protocol Support #311

merged 24 commits into from
Jan 17, 2020

Conversation

flycash
Copy link
Member

@flycash flycash commented Jan 9, 2020

What this PR does:
Merge the feature/grpc into develop
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

  1. Check the code coverage
  2. Test it
    Does this PR introduce a user-facing change?:
Now we support grpc!

@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #311 into develop will increase coverage by 0.27%.
The diff coverage is 79.21%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #311      +/-   ##
===========================================
+ Coverage    66.58%   66.85%   +0.27%     
===========================================
  Files          113      118       +5     
  Lines         7239     7467     +228     
===========================================
+ Hits          4820     4992     +172     
- Misses        1947     1977      +30     
- Partials       472      498      +26
Impacted Files Coverage Δ
filter/filter_impl/tps/tps_limiter_mock.go 0% <ø> (ø)
...ter/handler/rejected_execution_handler_only_log.go 100% <ø> (ø)
filter/filter_impl/graceful_shutdown_filter.go 80.64% <ø> (ø)
config_center/configurator/override.go 75.4% <ø> (ø) ⬆️
filter/handler/rejected_execution_handler_mock.go 0% <ø> (ø)
filter/filter_impl/generic_filter.go 67.69% <ø> (ø)
...l/tps/tps_limit_thread_safe_fix_window_strategy.go 100% <ø> (ø)
filter/filter_impl/tps_limit_filter.go 88.23% <ø> (ø)
filter/filter_impl/execute_limit_filter.go 73.8% <ø> (ø)
config/method_config.go 50% <ø> (ø) ⬆️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 979acb2...1c0e422. Read the comment docs.

flycash and others added 2 commits January 10, 2020 11:18
Merge develop into feature/grpc and resolve conflicts
@AlexStocks
Copy link
Contributor

Perfect. LGTM

Copy link
Contributor

@fangyincheng fangyincheng left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -239,6 +241,7 @@ func (l *ZkEventListener) listenDirEvent(zkPath string, listener remoting.DataLi

//listen sub path recursive
go func(zkPath string, listener remoting.DataListener) {
fmt.Printf("zkpath: %v \n", zkPath)
l.listenDirEvent(zkPath, listener)
Copy link
Contributor

Choose a reason for hiding this comment

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

rm fmt.Printf

@@ -29,7 +29,7 @@ import (
// Extension - protocol
type Protocol interface {
Export(invoker Invoker) Exporter
Refer(url common.URL) Invoker
Refer(url common.URL, impl interface{}) Invoker
Destroy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any other solution rather than add a parameter? Too many code changed..
And it's also not commonly used except for grpc.

Mod: modidfy Refer params and add licence
Copy link
Contributor

@hxmhlt hxmhlt left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexStocks AlexStocks merged commit 3897328 into develop Jan 17, 2020
@AlexStocks AlexStocks deleted the feature/grpc branch January 17, 2021 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants