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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change the default batch size to 1024 #1396

Merged
merged 4 commits into from Mar 17, 2021
Merged

Change the default batch size to 1024 #1396

merged 4 commits into from Mar 17, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Feb 22, 2021

馃摂 Description

Since the default partitions size is a power of 2 it makes sense to do the same for the default batch size.

馃摑 Checklist

  • All user-facing changes have changelog entries.

@tobim tobim added the maintenance Tasks for keeping up the infrastructure label Feb 22, 2021
@tobim tobim requested a review from a team February 22, 2021 14:14
mavam
mavam reviewed Feb 22, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@tobim tobim force-pushed the topic/power-of-2-batch-size branch from 48ef89d to be8244a Compare February 24, 2021 14:40
dominiklohmann
dominiklohmann reviewed Feb 24, 2021
View reviewed changes
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I don't think this change makes any sense unless you also change vast.metrics.self-sink.slice-size, which, as I've just noticed, always uses the default value, as libvast/src/accountant/config.cpp has a spelling error in the key and writes slice as slize in two places.

dominiklohmann
dominiklohmann reviewed Feb 24, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@tobim tobim force-pushed the topic/power-of-2-batch-size branch from be8244a to a12b510 Compare February 24, 2021 14:46
@tobim
Copy link
Member Author

tobim commented Feb 24, 2021

I also changed the slice size of the accountant to a power of 2.

@tobim tobim force-pushed the topic/power-of-2-batch-size branch from 2298bc7 to 1409ea8 Compare February 24, 2021 18:55
lava
lava approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Looks good to me, the only thing I noticed was a typo in the word "typo" in one of the commit messages :D (Fix a type in the accountant config ) But I think it's fine to just ignore this one.

@tobim tobim force-pushed the topic/power-of-2-batch-size branch from 1409ea8 to a42df74 Compare March 17, 2021 11:52
@tobim tobim enabled auto-merge March 17, 2021 11:53
@tobim tobim changed the base branch from master to story/ch23612/meta-index-erase March 17, 2021 12:34
@tobim tobim force-pushed the topic/power-of-2-batch-size branch from a42df74 to 622ec41 Compare March 17, 2021 12:40
@tobim tobim merged commit 517aba3 into story/ch23612/meta-index-erase Mar 17, 2021
@tobim tobim deleted the topic/power-of-2-batch-size branch March 17, 2021 12:40
@tobim tobim restored the topic/power-of-2-batch-size branch March 17, 2021 13:51
@tobim tobim mentioned this pull request Mar 17, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavam mavam mavam left review comments

@dominiklohmann dominiklohmann dominiklohmann left review comments

@lava lava lava approved these changes

Assignees

@lava lava

Labels
maintenance Tasks for keeping up the infrastructure
Projects
None yet
Milestone
No milestone
4 participants
@tobim @lava @mavam @dominiklohmann