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

Switch index behavior when running out of workers #1210

Merged
merged 5 commits into from
Dec 4, 2020

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Dec 3, 2020

📔 Description

This fixes a logic error. To reproduce, add the following assertion on current master:

diff --git a/libvast/src/system/index.cpp b/libvast/src/system/index.cpp
index 2cfd19028..6bc869635 100644
--- a/libvast/src/system/index.cpp
+++ b/libvast/src/system/index.cpp
@@ -197,6 +197,7 @@ bool index_state::worker_available() {
 }

 caf::actor index_state::next_worker() {
+  VAST_ASSERT(worker_available());
   auto result = std::move(idle_workers.back());
   idle_workers.pop_back();
   return result;

Compile, then start a node. Ideally, import lots of data—I tested this by ingesting the whole ICTF data set.

Now, start multiple export commands as background processes that don't have much work to do, e.g.,

repeat 100; do vast export -n 100 null '#type ni ""' &; done

The newly added assertion sometimes triggers.

The change made in this PR fixes the behavior switching.

📝 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

Reason about correct behavior. I've requested @lava as our resident index expert for review.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Dec 3, 2020
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, reviewed this offline in slack call.

@lava lava merged commit 32cee1b into master Dec 4, 2020
@lava lava deleted the topic/no-index-workers branch December 4, 2020 10:09
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