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

Allow fine-grained meta index configuration #2065

Merged
merged 7 commits into from Mar 21, 2022
Merged

Allow fine-grained meta index configuration #2065

merged 7 commits into from Mar 21, 2022

Conversation

lava
Copy link
Member

@lava lava commented Feb 4, 2022 •
edited

馃摑 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

馃幆 Review Instructions

Review this pull request commit-by-commit.

Base automatically changed from topic/index-config to master February 4, 2022 18:28
@dominiklohmann dominiklohmann added the feature New functionality label Feb 7, 2022
@lava lava force-pushed the story/sc-31434/meta-index-fprate-config branch 2 times, most recently from 5019fc2 to 018f263 Compare March 11, 2022 18:31
@lava lava force-pushed the story/sc-31434/meta-index-fprate-config branch 2 times, most recently from 7e17edb to 56d5d1a Compare March 15, 2022 16:57
@lava lava changed the title wip: Allow fine-grained meta index configuration Allow fine-grained meta index configuration Mar 15, 2022
@lava lava marked this pull request as ready for review March 15, 2022 17:25
@lava lava force-pushed the story/sc-31434/meta-index-fprate-config branch from 56d5d1a to 1b804f3 Compare March 15, 2022 17:28
dominiklohmann
dominiklohmann reviewed Mar 17, 2022
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.

This whole PR needs a changelog entry and a docs update. We should also clarify in there how/whether this affects the #skip and #index=hash attributes.

I had some general questions inline, most importantly on the configuration section.

libvast/src/system/spawn_index.cpp Outdated Show resolved Hide resolved
libvast/src/system/index.cpp Show resolved Hide resolved
libvast/src/system/index.cpp Outdated Show resolved Hide resolved
libvast/src/system/spawn_index.cpp Outdated Show resolved Hide resolved
tobim
tobim requested changes Mar 17, 2022
View reviewed changes
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

This looks alright to me, but I would like to see a unit test that verifies that synopses are constructed with the correct parameters.

libvast/src/partition_synopsis.cpp Outdated Show resolved Hide resolved
vast/integration/reference/basiccommand/step_00.ref Outdated Show resolved Hide resolved
@lava lava force-pushed the story/sc-31434/meta-index-fprate-config branch from 5229673 to 541fb2f Compare March 20, 2022 22:02
@lava lava force-pushed the story/sc-31434/meta-index-fprate-config branch from 7869ec5 to a98b63e Compare March 21, 2022 15:30
@lava lava force-pushed the story/sc-31434/meta-index-fprate-config branch from a98b63e to 8da1c61 Compare March 21, 2022 15:31
tobim
tobim approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

@lava lava added the bug Incorrect behavior label Mar 21, 2022
@lava lava merged commit a7d502b into master Mar 21, 2022
@lava lava deleted the story/sc-31434/meta-index-fprate-config branch March 21, 2022 18:26
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

@tobim tobim tobim approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior feature New functionality
Projects
None yet
Milestone
No milestone
4 participants
@lava @mavam @tobim @dominiklohmann