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

runtime: support unix domain socket #3529

Merged
merged 3 commits into from
Aug 20, 2021
Merged

runtime: support unix domain socket #3529

merged 3 commits into from
Aug 20, 2021

Conversation

daixiang0
Copy link
Member

@daixiang0 daixiang0 commented Aug 9, 2021

Signed-off-by: Long long0dai@foxmail.com

Description

Support unix domain socket.

Issue reference

Please reference the issue this PR will close: #2864

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@daixiang0
Copy link
Member Author

daixiang0 commented Aug 9, 2021

after build:

  1. run with hello-world example in quick-start
    daprd --app-id nodeapp --app-protocol http --components-path /root/.dapr/components --metrics-port 39497 --dapr-http-max-request-size -1 --app-port 3000 --placement-host-address localhost:50005 --config /root/.dapr/config.yaml --enable-domain-socket
  2. test
    curl --unix-socket /tmp/nodeapp-http.socket http://unix/v1.0/state/statestore/order

@yaron2
Copy link
Member

yaron2 commented Aug 9, 2021

I'm really happy to see this PR landing. Notice tests are failing.

@daixiang0 daixiang0 changed the title runtime: support unix domain socket [WIP] runtime: support unix domain socket Aug 9, 2021
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #3529 (90c855a) into master (a30488d) will decrease coverage by 0.14%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3529      +/-   ##
==========================================
- Coverage   60.51%   60.37%   -0.15%     
==========================================
  Files          97       97              
  Lines        8760     8784      +24     
==========================================
+ Hits         5301     5303       +2     
- Misses       3070     3092      +22     
  Partials      389      389              
Impacted Files Coverage Δ
pkg/channel/http/http_channel.go 67.02% <ø> (ø)
pkg/grpc/server.go 17.60% <0.00%> (-1.21%) ⬇️
pkg/http/config.go 0.00% <0.00%> (ø)
pkg/http/server.go 55.20% <0.00%> (-1.79%) ⬇️
pkg/runtime/cli.go 4.41% <0.00%> (-0.04%) ⬇️
pkg/runtime/runtime.go 53.82% <0.00%> (-0.47%) ⬇️
pkg/grpc/config.go 100.00% <100.00%> (ø)
pkg/runtime/config.go 100.00% <100.00%> (ø)

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 1f9adee...90c855a. Read the comment docs.

@daixiang0
Copy link
Member Author

daixiang0 commented Aug 13, 2021

@yaron2 @artursouza not sure is it necessary for all e2e tests, so no e2e tests for now, please review codes first, thanks.

@daixiang0 daixiang0 changed the title [WIP] runtime: support unix domain socket runtime: support unix domain socket Aug 13, 2021
Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

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

LGTM overall. This would need changes in the CLI, docs and SDKs too.

pkg/grpc/server.go Outdated Show resolved Hide resolved
pkg/http/server.go Outdated Show resolved Hide resolved
pkg/runtime/runtime.go Outdated Show resolved Hide resolved
@artursouza artursouza self-assigned this Aug 14, 2021
@daixiang0
Copy link
Member Author

@artursouza I think CLI changes need to wait this merged if there is any e2e based on daprd, docs changes as well, SDK is the last step while I do not familiar with other languages, I will create issues to track it.

@daixiang0
Copy link
Member Author

sorry for force push since flaky tests, if there is a better way to re-run tests, please tell me, thanks.

Signed-off-by: Long <long0dai@foxmail.com>
daixiang0 added a commit to daixiang0/dapr-docs that referenced this pull request Aug 19, 2021
daixiang0 added a commit to daixiang0/dapr-docs that referenced this pull request Aug 19, 2021
@greenie-msft
Copy link
Contributor

@artursouza, should this be included in the 1.4 milestone?

@artursouza
Copy link
Member

@artursouza, should this be included in the 1.4 milestone?

I prefer to add community contributions to the milestone when they get merged.

@artursouza
Copy link
Member

Please, create tracking issues in CLI and SDKs.

@artursouza
Copy link
Member

@yaron2 @greenie-msft @orizohar For this feature to be announced, we would need to have support in at least CLI and one SDK.

@artursouza artursouza added the automerge Allows DaprBot to automerge and update PR if all approvals are in place label Aug 19, 2021
@artursouza artursouza added this to the v1.4 milestone Aug 19, 2021
@yaron2
Copy link
Member

yaron2 commented Aug 20, 2021

@yaron2 @greenie-msft @orizohar For this feature to be announced, we would need to have support in at least CLI and one SDK.

Ok, but why? For example, cant users directly call via the socket if using their own gRPC client?

@artursouza
Copy link
Member

@yaron2 @greenie-msft @orizohar For this feature to be announced, we would need to have support in at least CLI and one SDK.

Ok, but why? For example, cant users directly call via the socket if using their own gRPC client?

Yes, but we should make it a first class feature with flags from CLI and config in the SDKs. Otherwise, people might not use it.

@yaron2
Copy link
Member

yaron2 commented Aug 20, 2021

@yaron2 @greenie-msft @orizohar For this feature to be announced, we would need to have support in at least CLI and one SDK.

Ok, but why? For example, cant users directly call via the socket if using their own gRPC client?

Yes, but we should make it a first class feature with flags from CLI and config in the SDKs. Otherwise, people might not use it.

Ok, just making sure there isn't some special config that needs to be done for this, and that regular gRPC/HTTP clients can use this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Allows DaprBot to automerge and update PR if all approvals are in place
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Run app and sidecar separately without requiring hardcoded ports
4 participants