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

Increase Kafka container wait time for HTTP bindings examples #1054

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

artur-ciocanu
Copy link
Contributor

Description

Currently when running Java SDK examples for HTTP bindings it fails. It seems that using sleep: 5 to wait for Kafka container to be up and running is not enough. The change is to increase the sleep value from 5 to 20. This is not ideal, but it proved to work when running:

mm.py ./src/main/java/io/dapr/examples/bindings/http/README.md

on local box.

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

@artur-ciocanu artur-ciocanu requested review from a team as code owners June 20, 2024 14:31
Signed-off-by: Artur Ciocanu <ciocanu@adobe.com>
@artur-ciocanu
Copy link
Contributor Author

artur-ciocanu commented Jun 20, 2024

@artursouza and @cicoyle after I have adjusted the sleep for Kafka container the Auto Validate Examples workflow passed. Here is a sample run: https://github.com/artur-ciocanu/java-sdk/actions/runs/9599423378/job/26473113165.

Could you please take a look and let me know if you think it is OK to increase the sleep.

@artur-ciocanu artur-ciocanu mentioned this pull request Jun 20, 2024
3 tasks
@salaboy
Copy link
Contributor

salaboy commented Jun 20, 2024

Increasing the sleep usually highlights an issue, but it might be the case that for this test the initialisation is taking way much more time. I will defer to @artursouza and @cicoyle to validate

Copy link
Contributor

@cicoyle cicoyle left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, in looking at the logs I see duration_seconds: 16.4 for this step: Setup kafka container. I'm uncertain this fixes all the issues from this PR's autovalidate examples CI step, but we shall see

@artur-ciocanu
Copy link
Contributor Author

artur-ciocanu commented Jun 20, 2024

Increasing the sleep usually highlights an issue, but it might be the case that for this test the initialisation is taking way much more time. I will defer to @artursouza and @cicoyle to validate

@salaboy I totally agree. I am not sure if https://github.com/dapr/mechanical-markdown aka mm.py script that is used to run the tests from the examples folder, has a way to wait for a "signal" or something from the Docker Compose so it doesn't use sleep. @cicoyle any thoughts?

@artursouza
Copy link
Member

The idea would be to have the sidecar to retry the init for some time before crashing. For this scope, timeout bump works.

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.79%. Comparing base (d759c53) to head (4ce7412).
Report is 19 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1054      +/-   ##
============================================
+ Coverage     76.91%   77.79%   +0.88%     
+ Complexity     1592     1570      -22     
============================================
  Files           145      144       -1     
  Lines          4843     4797      -46     
  Branches        562      563       +1     
============================================
+ Hits           3725     3732       +7     
+ Misses          821      780      -41     
+ Partials        297      285      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@artur-ciocanu
Copy link
Contributor Author

@artursouza and @cicoyle it seems that everything is 👍. Could you please merge the PR?

@artursouza artursouza merged commit 9048bc1 into dapr:master Jun 20, 2024
12 checks passed
@artur-ciocanu artur-ciocanu deleted the fix-http-bindings-examples branch June 27, 2024 09:56
@cicoyle cicoyle added this to the v1.12 milestone Jul 23, 2024
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.

4 participants