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

fix: Retry configuring Couchbase on HttpIOException #1064

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

mgroves
Copy link
Contributor

@mgroves mgroves commented Dec 5, 2023

What does this PR do?

Added RetryHandler handler to the configuration, per discussion in issue #1062

Why is it important?

This is necessary because of the way Couchbase startup works very asynchronously. Various parts are not guaranteed to be ready in any particular order. This was causing 504 errors when making HTTP requests prematurely. A simple retry takes care of this.

Related issues

This should closes #1062

How to test this PR

Difficult to test, since the underlying issue is intermittent. However, the existing Couchbase tests passed with this updated code.

Follow-ups

My main concern is that the retry logic is pretty simple. If this issue crops up again, I would suggest lengthening the delay, increasing number of retries, and/or adding some kind of retry backoff period, and see if any of those help.

Copy link

netlify bot commented Dec 5, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit bee9f0a
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/6570a46366dd17000833e367
😎 Deploy Preview https://deploy-preview-1064--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mgroves
Copy link
Contributor Author

mgroves commented Dec 5, 2023

The template said to add a label, but I don't seem to have the ability to do that. I just see "none yet" and no gear icon (yet).

@HofmeisterAn
Copy link
Collaborator

Thanks for creating the PR. I will look at it tomorrow. I would like to include a comment that explains why the retry delegation is necessary. But for today, I am done.

The template said to add a label, but I don't seem to have the ability to do that. I just see "none yet" and no gear icon (yet).

Oh, interesting. I was not aware of that. I will look into it. Thanks.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thank you for creating the issue, initiating the discussion, and finally the PR 🙌. I think it is a pragmatic fix for the issue you encountered. Thanks!

The template said to add a label, but I don't seem to have the ability to do that. I just see "none yet" and no gear icon (yet).

It looks like this requires additional permissions. I will remove the section from the template.

@HofmeisterAn HofmeisterAn changed the title Adding HTTP retry to Couchbase config fix: Retry configuring Couchbase on HttpIOException Dec 6, 2023
@HofmeisterAn HofmeisterAn added the bug Something isn't working label Dec 6, 2023
@mgroves
Copy link
Contributor Author

mgroves commented Dec 6, 2023

And thank YOU for being so responsive, and providing excellent guidance :)

@HofmeisterAn HofmeisterAn merged commit e66a51f into testcontainers:develop Dec 6, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Intermittant "The response ended prematurely" with Couchbase
2 participants