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

Support static Properties overrides on DaprClientBuilder #1097

Merged
merged 16 commits into from
Aug 24, 2024

Conversation

salaboy
Copy link
Contributor

@salaboy salaboy commented Aug 9, 2024

Description

After discussing this with @cicoyle and @artursouza I've created this PR to provide a way to override variables that are set using System properties and environments.

Issue reference

Fixes #1096

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

@salaboy
Copy link
Contributor Author

salaboy commented Aug 9, 2024

@artursouza @cicoyle the issue in the pipeline seems to a flaky test more than an issue caused by the changes in this PR
Can we retrigger?

@salaboy salaboy force-pushed the prop branch 2 times, most recently from 0eb4e69 to d769850 Compare August 9, 2024 11:54
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

@salaboy everything looks good!

pom.xml Outdated Show resolved Hide resolved
@cicoyle
Copy link
Contributor

cicoyle commented Aug 12, 2024

This looks to be a little more than flaky:

MethodInvokeIT.testInvokeTimeout:95 Message: DEADLINE_EXCEEDED: context deadline exceeded does not start with: DEADLINE_EXCEEDED: deadline exceeded after ==> expected: <true> but was: <false>

I dont know that its related to this PR, but its consistent

@salaboy
Copy link
Contributor Author

salaboy commented Aug 13, 2024

@cicoyle now it is a different one: https://github.com/dapr/java-sdk/actions/runs/10318759933/job/28670836841?pr=1097

BindingIT.inputOutputBinding:49->BaseIT.startDaprApp:49->BaseIT.startDaprApp:68->BaseIT.startDaprApp:130 » IllegalState Could not find success criteria for command: dapr run --app-id bindingit-grpc-inputbindingservice --app-protocol http --config ./configurations/configuration.yaml --resources-path ./components --app-port 36851 --dapr-http-port 40149 --dapr-grpc-port 34267 -- mvn exec:java -D exec.mainClass=io.dapr.it.binding.http.InputBindingService -D exec.classpathScope=test -D exec.args="36851"

@artur-ciocanu
Copy link
Contributor

This looks to be a little more than flaky:


MethodInvokeIT.testInvokeTimeout:95 Message: DEADLINE_EXCEEDED: context deadline exceeded does not start with: DEADLINE_EXCEEDED: deadline exceeded after ==> expected: <true> but was: <false>

I dont know that its related to this PR, but its consistent

@cicoyle actually the test should be adjusted. I am not entirely sure how GRPC decides to fire a "StatusRuntimeException", but sometime it contains "DEADLINE context deadline ..." in other cases it contains "DEADLINE after timeout ...".

I think looking for "DEADLINE" should be enough.

Let me know your thoughts.

@salaboy
Copy link
Contributor Author

salaboy commented Aug 15, 2024

@artur-ciocanu @cicoyle @artursouza For some reason, I was experimenting with similar "different" error messages popping up. For example, if I run some tests from my IDE, I will get different error messages than if I run them from the terminal or in CI. I don't know why but it makes me think that error codes are not super deterministic, if I don't remember wrong @cicoyle did you worked on that?

@salaboy
Copy link
Contributor Author

salaboy commented Aug 15, 2024

@cicoyle @artursouza look at the issues now.. completely different and on setup ..
Can we merge this and solve any related consistent problems in the following PR that depends on this one?

Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

@salaboy the PR looks great, I have a left a tiny comment.

@artur-ciocanu
Copy link
Contributor

@artursouza and @cicoyle could you please review. Thank you.

@artur-ciocanu
Copy link
Contributor

@salaboy something is off ... GitHub says that there are Files changed 1,284. Could you please take a look.

@artur-ciocanu
Copy link
Contributor

@salaboy could you please rebase from master.

artursouza
artursouza previously approved these changes Aug 22, 2024
@salaboy
Copy link
Contributor Author

salaboy commented Aug 22, 2024

@artursouza is this going to be merged?

@artursouza
Copy link
Member

One test run is not passing. Automerge takes care of updating the branch and merging after all tests pass.

@salaboy
Copy link
Contributor Author

salaboy commented Aug 22, 2024 via email

artursouza and others added 6 commits August 23, 2024 16:47
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
* update dapr runtime + cli to latest rc

* update install url version

* add back v

* trying things for pom.xml

* Fix sdk-autogen proto code gen

* Use 1.14.0-rc.3 CLI for build.yaml

* debug scheduler connection.

* Update CLI to rc6 and runtime to rc4

* Update gRPC and proto dependency in sdk-tests too.

* Update to runtime RC6

* Update error message expectations in 1.15

---------

Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <artursouza.ms@outlook.com>
Co-authored-by: Cassie Coyle <cassie@diagrid.io>
Signed-off-by: salaboy <Salaboy@gmail.com>
artursouza and others added 9 commits August 23, 2024 16:47
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
…1109)

Signed-off-by: Artur Ciocanu <ciocanu@adobe.com>
Co-authored-by: Artur Ciocanu <ciocanu@adobe.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
Signed-off-by: salaboy <Salaboy@gmail.com>
@salaboy
Copy link
Contributor Author

salaboy commented Aug 23, 2024

@artursouza here we go again.. all tests are green now

@artursouza artursouza merged commit b808c92 into dapr:master Aug 24, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling static Properties to be overriden by the DaprClientBuilder
4 participants