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 file loader #3088

Merged
merged 26 commits into from Apr 24, 2023
Merged

Implement file loader #3088

merged 26 commits into from Apr 24, 2023

Conversation

Dakostu
Copy link
Member

@Dakostu Dakostu commented Apr 19, 2023

This PR implements a file loader with an optional --timeout parameter that forwards regular and UDS file input to the next pipeline operator.

This loader also replaces the stdin loader.

@Dakostu Dakostu added feature New functionality refactoring Restructuring of existing code labels Apr 19, 2023
@Dakostu Dakostu requested a review from a team April 19, 2023 12:02
@Dakostu Dakostu force-pushed the topic/file-loader branch 4 times, most recently from f117e0d to 001e666 Compare April 19, 2023 13:41
@jachris
Copy link
Contributor

jachris commented Apr 19, 2023

You might want to consider overriding operator_base::infer_type_impl for load_operator as part of this PR. This is because the default implementation of infer_type_impl will instantiate the pipeline with a dummy generator, which calls vast::plugins::file::plugin::make_loader in this case. This will already try to open the file which can fail. However, perhaps this part of the pipeline is to be executed on another node where it would succeed.

For the loader, this effect is rather subtle, but for the saver you most definitely want to override it.

Edit: There is another alternative, which is to put the side-effects into the returned generator, such that they will not be executed as part of type checking.

@Dakostu Dakostu force-pushed the topic/file-loader branch 2 times, most recently from 1c8a2e5 to 1edb768 Compare April 19, 2023 14:49
@jachris jachris force-pushed the topic/connector-and-format-options branch from f83454f to c463109 Compare April 20, 2023 07:46
@Dakostu Dakostu force-pushed the topic/file-loader branch 3 times, most recently from 358e35e to ade716d Compare April 20, 2023 08:54
@Dakostu Dakostu marked this pull request as ready for review April 20, 2023 09:00
Copy link
Contributor

@jachris jachris left a comment

Choose a reason for hiding this comment

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

Nice, we are now very close to having a file loader 🎉

Sorry for writing so many comments – most of them are nits or questions. I would like to take one final look after things are clarified.

libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/src/detail/file_path_to_parser.cpp Outdated Show resolved Hide resolved
libvast/test/loader_plugin.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Show resolved Hide resolved
Base automatically changed from topic/connector-and-format-options to main April 20, 2023 17:46
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
libvast/builtins/connectors/file.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jachris jachris left a comment

Choose a reason for hiding this comment

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

Great. I think this will be complete after the open points have been addressed 🎉

@Dakostu Dakostu force-pushed the topic/file-loader branch 4 times, most recently from c46fce7 to e0bd40f Compare April 21, 2023 11:32
@Dakostu Dakostu enabled auto-merge April 21, 2023 11:35
@Dakostu Dakostu force-pushed the topic/file-loader branch 2 times, most recently from 8e17121 to e70320a Compare April 21, 2023 14:31
@Dakostu Dakostu merged commit eb4c4b8 into main Apr 24, 2023
41 checks passed
@Dakostu Dakostu deleted the topic/file-loader branch April 24, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality refactoring Restructuring of existing code
Projects
None yet
3 participants