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

Add support for partition-local stores #1514

Merged
merged 47 commits into from Apr 22, 2021
Merged

Add support for partition-local stores #1514

merged 47 commits into from Apr 22, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Apr 1, 2021 •
edited by dominiklohmann

📔 Description

This refactoring changes the protocol for the output path.
The EXPORTER is not an archive client any more but only talks to the index.

📝 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

By commit.

@tobim tobim added the refactoring Restructuring of existing code label Apr 1, 2021
@tobim tobim force-pushed the story/ch20246/event-store branch from 642bdc7 to 2e63458 Compare April 1, 2021 08:59
@tobim tobim changed the title Story/ch20246/event store Add support for parition-local stores Apr 1, 2021
@tobim tobim force-pushed the story/ch20246/event-store branch from 2e63458 to 0a62112 Compare April 1, 2021 09:16
@dominiklohmann dominiklohmann self-assigned this Apr 1, 2021
dominiklohmann
dominiklohmann reviewed Apr 1, 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.

This is great. I like the approach you're taking for this one, it's a good mix of flexible and minimally invasive. 🚀

I think extending vast.fbs.partition.v0 is the way to go, so these two commits can probably be squashed into one in the end. I also left some in-line comments.

libvast/vast/system/archive.hpp Outdated Show resolved Hide resolved
libvast/vast/system/actors.hpp Show resolved Hide resolved
libvast/vast/fbs/partition.fbs Outdated Show resolved Hide resolved
libvast/vast/fbs/partition.fbs Outdated Show resolved Hide resolved
libvast/vast/fbs/partition.fbs Outdated Show resolved Hide resolved
libvast/vast/system/evaluator.hpp Outdated Show resolved Hide resolved
libvast/src/system/evaluator.cpp Outdated Show resolved Hide resolved
libvast/src/system/evaluator.cpp Outdated Show resolved Hide resolved
libvast/test/system/evaluator.cpp Outdated Show resolved Hide resolved
libvast/vast/segment_store.hpp Show resolved Hide resolved
@tobim tobim force-pushed the story/ch20246/event-store branch from 0184d24 to 95e05bb Compare April 1, 2021 19:51
@dominiklohmann dominiklohmann changed the title Add support for parition-local stores Add support for partition-local stores Apr 1, 2021
@tobim tobim force-pushed the story/ch20246/event-store branch from 95e05bb to e4de9f9 Compare April 12, 2021 14:17
tobim
tobim commented Apr 12, 2021
View reviewed changes
libvast/vast/system/actors.hpp Outdated Show resolved Hide resolved
tobim
tobim commented Apr 12, 2021
View reviewed changes
libvast/vast/system/actors.hpp Outdated Show resolved Hide resolved
tobim
tobim commented Apr 12, 2021
View reviewed changes
libvast/vast/system/actors.hpp Outdated Show resolved Hide resolved
@tobim tobim force-pushed the story/ch20246/event-store branch 5 times, most recently from b8a3b31 to d87fce0 Compare April 15, 2021 12:04
@tobim tobim marked this pull request as ready for review April 15, 2021 12:51
tobim
tobim commented Apr 15, 2021
View reviewed changes
libvast/vast/table_slice.hpp Outdated Show resolved Hide resolved
tobim
tobim commented Apr 15, 2021
View reviewed changes
Copy link
Member Author

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

reviewed with @dominiklohmann.

libvast/vast/system/archive.hpp Outdated Show resolved Hide resolved
libvast/vast/system/archive.hpp Outdated Show resolved Hide resolved
libvast/src/table_slice.cpp Outdated Show resolved Hide resolved
libvast/src/system/archive.cpp Outdated Show resolved Hide resolved
libvast/src/system/archive.cpp Outdated Show resolved Hide resolved
libvast/src/system/archive.cpp Outdated Show resolved Hide resolved
libvast/vast/query_options.hpp Outdated Show resolved Hide resolved
libvast/vast/query_options.hpp Outdated Show resolved Hide resolved
libvast/vast/system/query_processor.hpp Show resolved Hide resolved
libvast/vast/system/query_processor.hpp Show resolved Hide resolved
@tobim tobim force-pushed the story/ch20246/event-store branch from d87fce0 to 389f5e3 Compare April 16, 2021 20:55
dominiklohmann
dominiklohmann reviewed Apr 20, 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.

Reviewed the last two commits, they mirror exactly what we talked about in the last review.

libvast/vast/query.hpp Outdated Show resolved Hide resolved
libvast/src/system/application.cpp Outdated Show resolved Hide resolved
@tobim tobim force-pushed the story/ch20246/event-store branch from 389f5e3 to cb5404f Compare April 20, 2021 21:43
@tobim tobim changed the base branch from epic/ch21972/ssp to master April 21, 2021 12:44
tobim added 15 commits April 21, 2021 20:32
@tobim
Use weak pointers to pass around the client
5167ee4
@tobim
Make it possible to export events with the correct ids
de95b2d 
The explorer needs this to deduplicate the events from multiple
second-stage queries.
@tobim
Adapt query_processor, counter & eraser
e2690aa
@tobim
Bring back the counter unit test
b17b301
@tobim
Bring back a test for the eraser
6beae28
@tobim
Bring back the query_supervisor unit test
8543de7
@tobim
Bring back the partition roundtrip test
42832b3
@tobim
Revert "Turn off the integration test for aging"
179fd1c 
This reverts commit d287261.
@tobim
Revert "Turn off integration testing of vast count"
4b3cb1a 
This reverts commit 3930047.
@tobim
Move the client handle into the query type
06ba352 
This allows us to use query as a strongly typed object that doesn't
need any additional context. The `weak_actor_pointer`s are not
used any more and will be removed from the actor interfaces in the
next commit.
@tobim
Remove the redundant client actor pointer
f46634d
@tobim
Separate id preservation from the historical option
95d7706
@tobim
Integrate review suggestions
894b771
@tobim
Fix formatting
7c9d9ab
@tobim
Minor comment cleanups
64d733d
@tobim tobim force-pushed the story/ch20246/event-store branch from b6fd917 to 64d733d Compare April 21, 2021 18:38
dominiklohmann
dominiklohmann reviewed Apr 22, 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.

Just some minor code nits. This is looking really good now.

Doing some final regression testing locally before approving.

libvast/src/system/archive.cpp Outdated Show resolved Hide resolved
libvast/src/system/archive.cpp Outdated Show resolved Hide resolved
libvast/src/system/evaluator.cpp Show resolved Hide resolved
libvast/vast/query.hpp Outdated Show resolved Hide resolved
libvast/vast/system/actors.hpp Show resolved Hide resolved
libvast/vast/system/actors.hpp Show resolved Hide resolved
libvast/vast/system/actors.hpp Show resolved Hide resolved
libvast/vast/table_slice.hpp Outdated Show resolved Hide resolved
libvast_test/vast/test/fixtures/actor_system.hpp Show resolved Hide resolved
tobim and others added 2 commits April 22, 2021 09:04
@tobim @dominiklohmann
Additional comment cleanups
d372524 
Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>
@tobim
Add comments to actor forward definitions
d9795d2
dominiklohmann
dominiklohmann reviewed Apr 22, 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.

Aside from the count performance regression while imports are running this is looking good. Looks like it'll be exactly 42 commits after all.

@tobim tobim mentioned this pull request Apr 22, 2021
Merged
3 tasks
dominiklohmann
dominiklohmann approved these changes Apr 22, 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.

Performance is looking much better now while imports are running. IMO, this is ready to ship. 🚀

@tobim
Fix short-circuiting in filter and count_matching
ea77704
@tobim
Prune meta predicates from the candidate checker
50f707e 
The hits for `#type` and `#field` extractors will always be exact, so
there is no need to double check. This does not apply to continuous
exports, because the candidate check is the sole filter in that case.
@tobim tobim enabled auto-merge April 22, 2021 14:49
@tobim tobim merged commit 56a43a7 into master Apr 22, 2021
@tobim tobim deleted the story/ch20246/event-store branch April 22, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiklohmann dominiklohmann dominiklohmann approved these changes

Assignees

@dominiklohmann dominiklohmann

Labels
refactoring Restructuring of existing code
Projects
None yet
Milestone
No milestone
2 participants
@tobim @dominiklohmann