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

Task-queue should only apply "runnable" concept to tasks #24780

Closed
gterzian opened this issue Nov 19, 2019 · 2 comments
Closed

Task-queue should only apply "runnable" concept to tasks #24780

gterzian opened this issue Nov 19, 2019 · 2 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Nov 19, 2019

When a pipeline is not "fully-active", the task-queue stores messages until the pipeline because "fully-active" again. The filtering is done at

self.store_task_for_inactive_pipeline(msg, &pipeline_id);

This is meant to implement the concept of runnable, which is how an event-loop selects tasks to run.

So, this concept only applies to tasks, yet the task-queue currently applies it to a T messages, which in practice is a message ultimately containing a script_runtime::CommonScriptMsg, and not all of these are tasks.

So we should only apply the "runnable" test to tasks, not all T, and the way to find out whether T contains a task, is by applying the same check the task-queue already applies in it's related "throttling" logic, by calling T::task_source_name

let task_source = match msg.task_source_name() {

If T doesn't have a task-source name, it's not a task, and it should always run whether it's associated document is fully-active or not.

@gterzian gterzian changed the title Task-queue should only take pipeline "fully-active" into account for tasks Task-queue should only apply "runnable" concept to tasks Nov 19, 2019
@gterzian
Copy link
Member Author

@gterzian gterzian commented Nov 19, 2019

Actually, that's not true.

The runnable check is only applied to tasks, because it's only applied to messages that have a pipeline_id:

if let Some(pipeline_id) = msg.pipeline_id() {

If you look at the various implementations of QueuedTaskConversion, pipeline_id returns None if the message doesn't contain a task:

fn pipeline_id(&self) -> Option<PipelineId> {

fn pipeline_id(&self) -> Option<PipelineId> {

fn pipeline_id(&self) -> Option<PipelineId> {

@gterzian gterzian closed this Nov 19, 2019
@gterzian
Copy link
Member Author

@gterzian gterzian commented Nov 19, 2019

If anything, this requires a comment, since it fooled the author of the actual code for a while, that makes clear that the pipeline_id call includes a check on whether the message is a task or not.

if let Some(pipeline_id) = msg.pipeline_id() {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.