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

cloudv2: Downscale the batch size #3193

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Jul 13, 2023

The backend has the limit set to 1k so it doesn't make sense to use a bigger value forcing the system to split a single batch in multiple jobs.

We are already overwriting the default value for most of the cases. This PR removes the requirement for setting the env var for doing the overwrite simplifing internal operations.

@codebien codebien added this to the v0.46.0 milestone Jul 13, 2023
@codebien codebien self-assigned this Jul 13, 2023
mstoykov
mstoykov previously approved these changes Jul 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023

Codecov Report

Merging #3193 (3071e74) into master (ac448b0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 3071e74 differs from pull request most recent head d4479c0. Consider uploading reports for the commit d4479c0 to get more accurate results

@@            Coverage Diff             @@
##           master    #3193      +/-   ##
==========================================
+ Coverage   72.85%   72.87%   +0.02%     
==========================================
  Files         256      256              
  Lines       19803    19804       +1     
==========================================
+ Hits        14428    14433       +5     
+ Misses       4474     4471       -3     
+ Partials      901      900       -1     
Flag Coverage Δ
ubuntu 72.81% <100.00%> (+0.01%) ⬆️
windows 72.71% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
output/cloud/output.go 75.33% <ø> (ø)
cloudapi/config.go 90.90% <100.00%> (ø)
output/cloud/expv2/output.go 65.74% <100.00%> (+0.19%) ⬆️

... and 2 files with indirect coverage changes

oleiade
oleiade previously approved these changes Jul 13, 2023
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.

Might be worth adding a comment explaining why we set it too 1000 directly in the code, as you explained here, but not blocking 👍🏻

@codebien
Copy link
Contributor Author

@oleiade good idea

@codebien codebien dismissed stale reviews from oleiade and mstoykov via c43ba7f July 13, 2023 15:24
mstoykov
mstoykov previously approved these changes Jul 13, 2023
@codebien
Copy link
Contributor Author

@olegbespalov added you to make sure you're aware of the change. We don't require anymore to set this option in our operations because the default already matches the requirement.

The backend has the limit set to 1k so it doesn't make sense to use a
bigger value forcing the system to split a single batch in multiple
jobs.
@codebien codebien merged commit 5f5e374 into master Jul 14, 2023
@codebien codebien deleted the cloudv2-adjust-batch-limit branch July 14, 2023 09:50
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.

5 participants