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 request context cancellation is ignored when retryBackoff (#539) #540

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

zhyu
Copy link
Contributor

@zhyu zhyu commented May 7, 2024

Fixes #539

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.99%. Comparing base (06a6dc8) to head (07c2b89).
Report is 41 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #540       +/-   ##
===========================================
+ Coverage   57.29%   67.99%   +10.70%     
===========================================
  Files         315      376       +61     
  Lines        9823     8862      -961     
===========================================
+ Hits         5628     6026      +398     
+ Misses       2902     1555     -1347     
+ Partials     1293     1281       -12     
Flag Coverage Δ
integration 60.35% <0.00%> (+9.51%) ⬆️
unit 15.37% <100.00%> (+2.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
opensearchtransport/opensearchtransport.go 87.16% <100.00%> (+2.00%) ⬆️

... and 272 files with indirect coverage changes

@dblock
Copy link
Member

dblock commented May 7, 2024

Thanks.

I'd love some additions to the docs on retries and back offs, can be a separate topic in https://github.com/opensearch-project/opensearch-go/tree/main/guides or https://github.com/opensearch-project/opensearch-go/blob/main/USER_GUIDE.md, but something generic enough on configuring the client?

Since you reference Elastic in #539, can you please confirm that you're not bringing in any non-APLv2 code in this project?

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

To fix DCO, run git commit -s --amend. Thanks.

@zhyu zhyu force-pushed the fix/539 branch 2 times, most recently from 7ff3db9 to c61f664 Compare May 8, 2024 08:34
@zhyu
Copy link
Contributor Author

zhyu commented May 8, 2024

I'd love some additions to the docs on retries and back offs

I added a very simple doc about configuring the client with retries and backoffs.

can you please confirm that you're not bringing in any non-APLv2 code in this project?

Yes, I can confirm that.

I also found the client will sleep when the last retry failed, which seems not expected. So I pushed another fix (8b2649c) for it. Please have a look. Thanks!

@dblock
Copy link
Member

dblock commented May 8, 2024

I also found the client will sleep when the last retry failed, which seems not expected. So I pushed another fix (8b2649c) for it.

Good find. This deserves its own test, too.

dblock
dblock previously approved these changes May 8, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks great. @Jakob3xD should take a look / merge, I am not super familiar with ways to do this in Go, I might be missing something.

Jakob3xD
Jakob3xD previously approved these changes May 8, 2024
Copy link
Collaborator

@Jakob3xD Jakob3xD left a comment

Choose a reason for hiding this comment

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

Only a small nit about the indent of the guide but good to go.

guides/retry_backoff.md Outdated Show resolved Hide resolved
@zhyu zhyu dismissed stale reviews from Jakob3xD and dblock via 2dcbb17 May 9, 2024 01:34
@dblock dblock requested a review from Jakob3xD May 13, 2024 20:52
dblock
dblock previously approved these changes May 16, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Merging, thanks!

I don't think we want an entire guide for retry and backoff. Maybe a "Configuration" doc that has a "Retries" section (and other configuration options)? @zhyu interested in (re)organizing things?

dblock
dblock previously approved these changes May 16, 2024
@dblock
Copy link
Member

dblock commented May 16, 2024

@zhyu Can you please take a quick look at the failing integration tests?

=== RUN   TestNodes/Stats/without_request
    api_nodes_test.go:182: 
        	Error Trace:	/home/runner/work/opensearch-go/opensearch-go/internal/test/helper.go:147
        	            				/home/runner/work/opensearch-go/opensearch-go/opensearchapi/api_nodes_test.go:182
        	Error:      	Should be empty, but was [{"op":"remove","path":"/nodes/QR8UfvZLSXmNJrK8ZCf0Pg/indices/search/search_idle_reactivate_count_total"}]
        	Test:       	TestNodes/Stats/without_request

I am not sure it's related, but could be. If it's a flake, check whether we already have an issue for it / open a new one.

@Jakob3xD
Copy link
Collaborator

I am not sure it's related, but could be. If it's a flake, check whether we already have an issue for it / open a new one.

The tests are failing because the new opensearch release adds a new field. Not related to the PR.

@Jakob3xD Jakob3xD merged commit 4fb1702 into opensearch-project:main Jul 1, 2024
55 checks passed
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.

[BUG] Request context cancellation is ignored when retryBackoff is configured
3 participants