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: add pass through proxy factory #1081

Merged
merged 14 commits into from
Apr 3, 2021

Conversation

LaurenceLiZhixin
Copy link
Contributor

What this PR does:
add pass through factory

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
pass through proxy invoker can invoke user defined service, and pass through all data to user defined service. User can wirte logic to deal with raw invocation data.
It is very useful to dapr-dubbo support.

Does this PR introduce a user-facing change?:


@LaurenceLiZhixin LaurenceLiZhixin changed the title feat: add pass through proxy facotry feat: add pass through proxy factory Mar 10, 2021
@@ -50,7 +50,8 @@ const (
PROTOCOL_KEY = "protocol"
PATH_SEPARATOR = "/"
//DUBBO_KEY = "dubbo"
SSL_ENABLED_KEY = "ssl-enabled"
SSL_ENABLED_KEY = "ssl-enabled"
ParameterTypeKey = "parameter-type-names" // ParameterType key used in dapr, to tranfer rsp param type
Copy link
Member

Choose a reason for hiding this comment

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

Maybe follow the codeSSL_ENABLED_KEY .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

var args [][]byte
if len(arguments) == 0 {
args = [][]byte{}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

We can reduce a if-else branch by

if len(arguments) > 0 {
	args = make([][]byte, 0, len(arguments))
	for _, arg := range arguments {
		if v, ok := arg.([]byte); ok {
			args = append(args, v)
		} else {
			result.Err = perrors.New("the param type is not []byte")
			return result
		}
	}
}

}
method := srv.Method()["Service"]

in := []reflect.Value{srv.Rcvr()}
Copy link
Member

Choose a reason for hiding this comment

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

Init with length in := make([]reflect.Value, 5).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

replyv := returnValues[0]
retErr = returnValues[1].Interface()

if retErr != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more appropriate.

	if retErr != nil {
		result.SetError(retErr.(error))
                return result
	}
	if replyv.IsValid() && (replyv.Kind() != reflect.Ptr || replyv.Kind() == reflect.Ptr && replyv.Elem().IsValid()) {
		result.SetResult(replyv.Interface())
	}
	return result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

logger.Infof("selector add service url{%s}", event.Service)
// FIXME: routers are built in every address notification?
dir.configRouters()
if event.Service != nil {
Copy link
Member

Choose a reason for hiding this comment

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

if event != nil && event.Service != nil

@LaurenceLiZhixin LaurenceLiZhixin changed the title feat: add pass through proxy factory Ftr: add pass through proxy factory Mar 14, 2021
@cityiron cityiron added this to the v3.0.0 milestone Mar 14, 2021
README.md Outdated
@@ -177,7 +177,7 @@ If you are willing to do some code contributions and document contributions to [

## Community

If u want to communicate with our community, pls scan the following [dubbobo Ding-Ding QR code](https://mmbiz.qpic.cn/mmbiz_jpg/yvBJb5IiafvnHVBdtia30dxA2hKotr9DEckWsZ7aOJcDWDaSVMGwLmYv8GRgIQtqb4C2svicp8nVkMmGy7yKC5tyA/640?wx_fmt=jpeg&tp=webp&wxfrom=5&wx_lazy=1&wx_co=1) or search our commnity DingDing group code 31363295.
If u want to communicate with our community, pls scan the following dubbobo Ding-Ding QR code or search our commnity DingDing group code 31363295.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Ding-Ding " -> DingDing


var args [][]byte
if len(arguments) > 0 {
args = make([][]byte, 0, len(arguments))
Copy link
Contributor

Choose a reason for hiding this comment

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

the same comment as line 93: pls using gxbytes.AcquireBytes instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gxbytes.AcquireBytes() is used to get specific make([]byte, 0, size), not to make [][]byte.

@codecov-io
Copy link

codecov-io commented Mar 21, 2021

Codecov Report

Merging #1081 (9284e7b) into 3.0 (a347a04) will decrease coverage by 0.67%.
The diff coverage is 54.23%.

❗ Current head 9284e7b differs from pull request most recent head 77f9cea. Consider uploading reports for the commit 77f9cea to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              3.0    #1081      +/-   ##
==========================================
- Coverage   60.03%   59.36%   -0.68%     
==========================================
  Files         260      269       +9     
  Lines       12855    13441     +586     
==========================================
+ Hits         7718     7979     +261     
- Misses       4174     4475     +301     
- Partials      963      987      +24     
Impacted Files Coverage Δ
cluster/cluster_impl/available_cluster.go 100.00% <ø> (ø)
cluster/cluster_impl/zone_aware_cluster_invoker.go 64.28% <ø> (ø)
cluster/router/healthcheck/default_health_check.go 100.00% <ø> (+5.88%) ⬆️
cluster/router/healthcheck/factory.go 66.66% <0.00%> (ø)
cluster/router/tag/router_rule.go 89.47% <ø> (+12.20%) ⬆️
common/config/environment.go 51.72% <ø> (ø)
common/extension/config_post_processor.go 0.00% <0.00%> (ø)
common/extension/conn_checker.go 0.00% <0.00%> (ø)
common/extension/metadata_service.go 0.00% <0.00%> (ø)
common/proxy/proxy_factory/default.go 13.55% <0.00%> (ø)
... and 138 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 ca7f3a8...77f9cea. Read the comment docs.

Copy link
Contributor

@zhaoyunxing92 zhaoyunxing92 left a comment

Choose a reason for hiding this comment

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

approved these changes

@AlexStocks AlexStocks merged commit f39cc52 into apache:3.0 Apr 3, 2021
cityiron pushed a commit that referenced this pull request Apr 11, 2021
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.

7 participants