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.

Sign up for GitHub

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

Jump to bottom

output/cloudv2: Retry and flushers pool #3104

Merged
merged 4 commits into from
Jun 15, 2023
Merged

output/cloudv2: Retry and flushers pool #3104

merged 4 commits into from
Jun 15, 2023

Conversation

codebien
Copy link
Contributor

@codebien codebien commented May 30, 2023 •
edited
Loading

It adds two features:

  • HTTP requests retry
  • Pool of flushers so that if a flush operation is slow, the output can continue and not get stuck.

@codebien codebien added this to the v0.45.0 milestone May 30, 2023
@codebien codebien self-assigned this May 30, 2023
@codebien codebien mentioned this pull request May 30, 2023
Closed
Base automatically changed from cloud-v2-bucket-unix-time to master May 31, 2023 09:26
@codebien codebien modified the milestones: v0.45.0, v0.46.0 Jun 1, 2023
@codebien codebien mentioned this pull request Jun 8, 2023
Closed
@codebien codebien force-pushed the cloud-v2-retries branch 4 times, most recently from 510ba34 to 0b8af75 Compare June 8, 2023 15:37
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023 •
edited
Loading

Codecov Report

Merging #3104 (9b18044) into master (9254ce7) will increase coverage by 0.07%.
The diff coverage is 89.47%.

❗ Current head 9b18044 differs from pull request most recent head e1b7419. Consider uploading reports for the commit e1b7419 to get more accurate results

@@            Coverage Diff             @@
##           master    #3104      +/-   ##
==========================================
+ Coverage   73.80%   73.87%   +0.07%     
==========================================
  Files         243      243              
  Lines       18474    18492      +18     
==========================================
+ Hits        13634    13661      +27     
+ Misses       3969     3963       -6     
+ Partials      871      868       -3
Flag Coverage Δ
ubuntu 73.80% <89.47%> (+0.07%) ⬆️
windows 73.68% <89.47%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
output/cloud/output.go 74.14% <33.33%> (ø)
output/cloud/expv2/metrics_client.go 55.55% <80.00%> (-2.45%) ⬇️
output/cloud/expv2/output.go 86.84% <95.12%> (+2.50%) ⬆️
cloudapi/client.go 73.46% <100.00%> (+1.30%) ⬆️
output/cloud/expv2/flush.go 84.61% <100.00%> (ø)

... and 3 files with indirect coverage changes

@codebien codebien marked this pull request as ready for review June 8, 2023 15:55
@github-actions github-actions bot requested review from mstoykov and oleiade June 8, 2023 15:56
@codebien codebien requested review from imiric and removed request for oleiade June 8, 2023 16:08
@codebien codebien mentioned this pull request Jun 9, 2023
Merged
@codebien
Copy link
Contributor Author

codebien commented Jun 9, 2023

Added the fix for #3108 (comment)

imiric
imiric reviewed Jun 9, 2023
View reviewed changes
output/cloud/expv2/output.go Outdated Show resolved Hide resolved
output/cloud/expv2/output.go Outdated Show resolved Hide resolved
output/cloud/expv2/metrics_client.go Outdated Show resolved Hide resolved
output/cloud/expv2/metrics_client.go Outdated
)

for attempts := 1; retry; attempts++ {
retry, err = mc.retryableDo(r, attempts)
Copy link
Contributor

Choose a reason for hiding this comment

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

We only return the error of the last retry attempt, and others are lost. Usually this would be the same error, but not necessarily. Do we care about the intermediate errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the answer to this is "no", since the v2 output now uses the cloudapi.Client, which also doesn't care about this, and it's been that way since it was written in 2017.

output/cloud/expv2/output_test.go Outdated Show resolved Hide resolved
@codebien
cloudapi: Make sure to always read the response body
6984d5a 
For reusing the same connection the HTTP client requires that the  response body
has been read. Most of the time the current implementation handles it
but it would require to depend on Go's internal implementation that
doesn't sound optimal. Essentially, this change follows what the doc is currently
suggesting.
@codebien
output/cloud: Polish check for concurrency option
8e07e8c
@codebien codebien force-pushed the cloud-v2-retries branch 2 times, most recently from d1900f6 to 1aa8b83 Compare June 14, 2023 08:19
@codebien codebien requested review from mstoykov and imiric June 14, 2023 08:29
mstoykov
mstoykov reviewed Jun 14, 2023
View reviewed changes
output/cloud/expv2/flush.go Show resolved Hide resolved
output/cloud/expv2/output.go Outdated Show resolved Hide resolved
output/cloud/expv2/metrics_client.go Outdated Show resolved Hide resolved
mstoykov
mstoykov previously approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM 👏 🎉

output/cloud/expv2/metrics_client.go
Comment on lines +29 to +32
// Unfortunately, the cloudapi.Client works across different versions
// of the API, but it has the v1 harcoded so we need to trim the wrong path
// to be able to replace it with the correct one.
u := c.BaseURL()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be done in the cloudapi package?

As a separate PR?

Copy link
Contributor Author

@codebien codebien Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the same but I would avoid loading you folks with a new PR to review, but you're asking for it so let's do it #3125 .

@codebien codebien requested a review from mstoykov June 15, 2023 08:40
mstoykov
mstoykov previously approved these changes Jun 15, 2023
View reviewed changes
imiric
imiric previously approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Good that this is now using the cloudapi.Client 👍

output/cloud/expv2/metrics_client.go Outdated Show resolved Hide resolved
@codebien codebien dismissed stale reviews from imiric and mstoykov via 79be0d6 June 15, 2023 10:02
imiric
imiric previously approved these changes Jun 15, 2023
View reviewed changes
@codebien
cloud/expv2: Retry and concurrent flush
5e9fa81 
Use the cloudapi Client for supporting retryable requests. The flush is
now concurrent across different flush workers.
@codebien
cloud/expv2/integration: Updated the NewClient call
e1b7419
@imiric imiric mentioned this pull request Jun 15, 2023
Merged
@codebien codebien merged commit 2e5631f into master Jun 15, 2023
@codebien codebien deleted the cloud-v2-retries branch June 15, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstoykov mstoykov mstoykov approved these changes

@imiric imiric imiric approved these changes

Assignees

@codebien codebien

Labels
None yet
Projects
None yet
Milestone
v0.46.0
Development

Successfully merging this pull request may close these issues.
4 participants
@codebien @codecov-commenter @mstoykov @imiric