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

Implement concurrent pushes across batches #3206

Merged
merged 10 commits into from
Jul 24, 2023

Conversation

olegbespalov
Copy link
Collaborator

@olegbespalov olegbespalov commented Jul 18, 2023

What?

This change moves the concurrency from flushes to batches. That fixes issue #3192 and generally improves processing.

Why?

Even if the single network request to ingester is fast enough, executing them sequentially increases the time.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3192

@olegbespalov olegbespalov self-assigned this Jul 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2023

Codecov Report

Merging #3206 (087e16d) into master (703970e) will increase coverage by 0.55%.
The diff coverage is 100.00%.

❗ Current head 087e16d differs from pull request most recent head a2878f7. Consider uploading reports for the commit a2878f7 to get more accurate results

@@            Coverage Diff             @@
##           master    #3206      +/-   ##
==========================================
+ Coverage   72.68%   73.24%   +0.55%     
==========================================
  Files         259      259              
  Lines       19864    19889      +25     
==========================================
+ Hits        14439    14567     +128     
+ Misses       4521     4400     -121     
- Partials      904      922      +18     
Flag Coverage Δ
ubuntu 73.16% <92.15%> (+0.53%) ⬆️
windows 73.07% <96.07%> (+0.54%) ⬆️

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

Impacted Files Coverage Δ
output/cloud/expv2/flush.go 95.60% <100.00%> (+4.55%) ⬆️
output/cloud/expv2/output.go 74.23% <100.00%> (+1.39%) ⬆️

... and 2 files with indirect coverage changes

@olegbespalov olegbespalov force-pushed the cloudv2-concurrency-per-batches branch 2 times, most recently from 16be249 to e2f4dc6 Compare July 20, 2023 12:32
@olegbespalov olegbespalov force-pushed the cloudv2-concurrency-per-batches branch from e2f4dc6 to 459cac9 Compare July 20, 2023 12:37
@olegbespalov olegbespalov changed the title Cloudv2 concurrency per batches Implement concurrent flush across batches Jul 20, 2023
@olegbespalov olegbespalov changed the title Implement concurrent flush across batches Implement concurrent pushes across batches Jul 20, 2023
@olegbespalov olegbespalov marked this pull request as ready for review July 20, 2023 13:44
@olegbespalov olegbespalov added this to the v0.46.0 milestone Jul 20, 2023
Copy link
Collaborator

@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, but I would prefer if we can do some changes to some of the math ... mostly removing some.

output/cloud/expv2/flush.go Outdated Show resolved Hide resolved
output/cloud/expv2/flush.go Outdated Show resolved Hide resolved
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

LGTM too 🤝

Non-blocking on my part, but I share the same concern regarding the work dispatching to multiple workers as @mstoykov. I think the code could be simplified indeed, to have one input channel, and multiple workers reading from it concurrently, I find it might also make the logic easier to follow.

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

Copy link
Collaborator

@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 !

@mstoykov mstoykov merged commit 344c446 into master Jul 24, 2023
21 checks passed
@mstoykov mstoykov deleted the cloudv2-concurrency-per-batches branch July 24, 2023 14:29
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.

Concurrent flush across batches
5 participants