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

Refactor importer initialization #647

Merged
merged 10 commits into from
Nov 18, 2019
Merged

Refactor importer initialization #647

merged 10 commits into from
Nov 18, 2019

Conversation

tobim
Copy link
Member

@tobim tobim commented Nov 14, 2019

The IMPORTER used to be set up with a 2-step initialization process. Some necessary parameters were passed at the call to spawn and others are supplied by message from the TRACKER. This PR moves all essential parameters to the spawning invocation.

Additionally, this PR factors the logic of retrieving NODE components into the dedicated functions get_component(node_actor*) and get_node_component(scoped_actor&, actor) to further decouple the TRACKER from the other actors (it is an implementation detail of the NODE and should not leak through the interface).

TODO:

  • Use get_node_component() in source_command.

@tobim tobim added the bug Incorrect behavior label Nov 14, 2019
@tobim tobim requested a review from dominiklohmann November 14, 2019 09:42
@tobim tobim force-pushed the story/ch9839 branch 2 times, most recently from 4351143 to 250b68e Compare November 15, 2019 14:41
@tobim tobim requested review from dominiklohmann and removed request for dominiklohmann November 15, 2019 14:43
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 like where this is headed, and only have a few minor things I'd like to see addressed. It seems to be working nicely.

Besides the inline comments, the only thing that needs to be addressed is the yet missing changelog entry.

libvast/src/system/spawn_consensus.cpp Outdated Show resolved Hide resolved
libvast/vast/system/importer.hpp Outdated Show resolved Hide resolved
libvast/src/system/sink_command.cpp Outdated Show resolved Hide resolved
libvast/vast/system/node_control.hpp Outdated Show resolved Hide resolved
libvast/vast/detail/array.hpp Outdated Show resolved Hide resolved
libvast/vast/system/node_control.hpp Outdated Show resolved Hide resolved
@dominiklohmann
Copy link
Member

Oh, one more thing I forgot to mention during the review: References to elements of structured bindings are not explicitly allowed by the C++17 standard (fixed for C++20, I believe). They happen to work with the gcc version used by CI, but fail to compile with clang8, which is why the CI build is failing on FreeBSD and macOS.

@tobim
Copy link
Member Author

tobim commented Nov 15, 2019

... They happen to work with the gcc version used by CI, but fail to compile with clang8, which is why the CI build is failing on FreeBSD and macOS.

I changed to code to not make use of structured bindings in source_command.cpp. This fixed the clang issue.

@tobim tobim force-pushed the story/ch9839 branch 2 times, most recently from 045b73c to cfc7c1e Compare November 17, 2019 21:21
dominiklohmann
dominiklohmann previously approved these changes Nov 18, 2019
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.

lgtm

@dominiklohmann dominiklohmann merged commit 233c5b1 into master Nov 18, 2019
@dominiklohmann dominiklohmann deleted the story/ch9839 branch November 18, 2019 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants