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

Provide custom grpc/http client for DaprClientBuilder as in #683  #690

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

subash89
Copy link

Signed-off-by: Subash Gamage [email protected]

Description

This is to Enhance DaprClientBuilder to override gRPC and Http Client. Complete description and reference JIRA can be found at #683

This includes changes in DaprClientBuilder class to accept a managed grpc client as well as ok http client.

Issue reference

#683

Please reference the issue this PR will close: #[issue number]

Checklist

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

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@subash89 subash89 requested review from a team as code owners February 23, 2022 11:59
README.md Outdated

```java
DaprClientBuilder daprClientBuilder = new DaprClientBuilder();
DaprClient client = daprClientBuilder.withOkHttpClient(new OkHttpClient.Builder().readTimeout(5, TimeUnit.SECONDS)).build();
Copy link
Member

Choose a reason for hiding this comment

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

We actually plan to remove the dependency on OkHTTP. Adding that to our public methods will make that harder. Why can't a custom gRPC or HTTP client be implemented with the existing interfaces?

Copy link
Author

Choose a reason for hiding this comment

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

The DaprClientBuilder create the channel internally and not allowing a way to override that outside.

README.md Outdated

```java
DaprClientBuilder daprClientBuilder = new DaprClientBuilder();
ManagedChannelBuilder managedChannelBuilder = Grpc.newChannelBuilder("localhost:8000", TlsChannelCredentials.create())
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. Which are the most common settings for a managed channel? Can this be done via additional java properties?

Copy link
Author

Choose a reason for hiding this comment

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

Channel has everything that matters and as I know we cannot control them via out of the box sys properties. Example using my own managed executor service

@subash89
Copy link
Author

subash89 commented Mar 5, 2022

@artursouza I updated the PR with new changes and reverted the old commit. New change is minimal as I provided a easy way to extend the builder class. Because maintaining a two Builder classes is hard to maintain as code is duplicated.
Let me know your thoughts. And I did not touch http builder yet as you said okhttp will go away. Once we have that finalized, I will add capability to override http client as well.

Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

@subash89 subash89 force-pushed the feature/extend-dapr-client branch from caffa08 to b7e364c Compare March 7, 2022 07:36
@subash89
Copy link
Author

subash89 commented Mar 7, 2022

Looks like it didn't like my reverse commit. I followed
git checkout
git commit --amend --no-edit --signoff
git push --force-with-lease
@mukundansundar Is it looks good now ? (How to make sure DCO is all set on a given pull request ?)

@mukundansundar
Copy link
Contributor

@subash89 Can you rebase all your commits into a single commit ? Also could you fix the check style violations in the PR?

@subash89
Copy link
Author

subash89 commented Mar 8, 2022

@mukundansundar
If you mean to say squash commits, I cannot squash them into one as I already pushed them to the branch. If not can you suggest how can I do that ? Or I can do a new PR with only this change.

This is my first PR, where to find checkstyle report inorder to fix those.

```java

DaprExtensibleClientBuilder daprClientBuilder = new DaprExtensibleClientBuilder();
ManagedChannelBuilder managedChannelBuilder = Grpc.newChannelBuilder("localhost:8000", TlsChannelCredentials.create())
Copy link
Member

Choose a reason for hiding this comment

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

Please, use properties like Properties.SIDECAR_IP.get() to make this portable when users run using a different host or port in standalone mode.

/**
* A builder for the DaprClient.
*/
public class DaprExtensibleClientBuilder extends DaprClientBuilder{
Copy link
Member

Choose a reason for hiding this comment

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

I would make this a GRPC specific builder and force the GRPC protocol, even if property is for HTTP.

With the current design, it would be possible for the code to set the GRPC managed channel and a Java property force the client built to be the HTTP instance.

Copy link
Author

Choose a reason for hiding this comment

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

Make sense.

@subash89
Copy link
Author

@artursouza meantime I update the PR, can you pls suggest a way to build this on a mac or some easy way to find out what are the checkstyle violaitons are.

@subash89
Copy link
Author

@artursouza Updated the PR. please let me know.

@subash89 subash89 force-pushed the feature/extend-dapr-client branch from fe6c9d7 to 5d985b4 Compare March 18, 2022 14:31
pravinpushkar and others added 13 commits March 18, 2022 10:31
* Adding javadocs to some Domain classes

Signed-off-by: pravinpushkar <[email protected]>

* Fixing code coverage

Signed-off-by: pravinpushkar <[email protected]>

* Incorporating review comments

Signed-off-by: pravinpushkar <[email protected]>
Signed-off-by: Subash Gamage <[email protected]>
Signed-off-by: Mukundan Sundararajan <[email protected]>

Co-authored-by: Artur Souza <[email protected]>
Signed-off-by: Subash Gamage <[email protected]>
…off-by: Subash Gamage <[email protected]>"

This reverts commit 1d88c76.

Signed-off-by: Subash Gamage <[email protected]>
Bumps [actions/setup-java](https://github.com/actions/setup-java) from 2.5.0 to 3.
- [Release notes](https://github.com/actions/setup-java/releases)
- [Commits](actions/setup-java@v2.5.0...v3)

---
updated-dependencies:
- dependency-name: actions/setup-java
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Subash Gamage <[email protected]>
Bumps [actions/github-script](https://github.com/actions/github-script) from 5 to 6.
- [Release notes](https://github.com/actions/github-script/releases)
- [Commits](actions/github-script@v5...v6)

---
updated-dependencies:
- dependency-name: actions/github-script
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Subash Gamage <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Subash Gamage <[email protected]>
* Add workflow and badge

Signed-off-by: Shubham Sharma <[email protected]>

* Encode space in URL

Signed-off-by: Shubham Sharma <[email protected]>
Signed-off-by: Subash Gamage <[email protected]>
* Add workflow and badge

Signed-off-by: Shubham Sharma <[email protected]>

* Encode space in URL

Signed-off-by: Shubham Sharma <[email protected]>

* Update FOSSA workflow condition

Signed-off-by: Shubham Sharma <[email protected]>
Signed-off-by: Subash Gamage <[email protected]>
* remove deprecated classes

Signed-off-by: Mukundan Sundararajan <[email protected]>

* remove builder class ref in ITs

Signed-off-by: Mukundan Sundararajan <[email protected]>

* fix setter in IT

Signed-off-by: Mukundan Sundararajan <[email protected]>
Signed-off-by: Subash Gamage <[email protected]>
Signed-off-by: pravinpushkar <[email protected]>
Signed-off-by: Subash Gamage <[email protected]>
@subash89
Copy link
Author

@artursouza any help appreciated.

@mukundansundar
Copy link
Contributor

@artursouza any help appreciated.

@subash89 Can you merge with latest master resolve conflicts and see if you are able to build it with the changes in master?

@subash89
Copy link
Author

@mukundansundar main problem I am having is to build this in a mac?
[ERROR] Failed to execute goal com.github.os72:protoc-jar-maven-plugin:3.11.4:run (default) on project dapr-sdk-autogen: Error extracting protoc for version 3.13.0: Unsupported platform: protoc-3.13.0-osx-x86_64.exe -> [Help 1]

@mukundansundar
Copy link
Contributor

mukundansundar commented Mar 24, 2022

@mukundansundar main problem I am having is to build this in a mac?
[ERROR] Failed to execute goal com.github.os72:protoc-jar-maven-plugin:3.11.4:run (default) on project dapr-sdk-autogen: Error extracting protoc for version 3.13.0: Unsupported platform: protoc-3.13.0-osx-x86_64.exe -> [Help 1]

Does this happen with the latest changes from master too?
Because I faced a similar issue with M1 Mac and I had to update some of the dependencies and use Rosetta enabled terminal for it to work.

@subash89
Copy link
Author

Yes in master too

@subash89
Copy link
Author

Can this be build on a linux box ?

@mukundansundar
Copy link
Contributor

Can this be build on a linux box ?

the build workflow is on a linux runner only

@amuthapriya4
Copy link

Please commit the changes for Managed channel to the release branch. With current version it going with default config.

@cicoyle
Copy link
Contributor

cicoyle commented Dec 21, 2023

@subash89 - mind resolving the conflicts?

@cicoyle
Copy link
Contributor

cicoyle commented Feb 19, 2024

gentle ping @subash89

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.

8 participants