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

Changed task_id to tracking_label #15443

Merged
merged 4 commits into from
Jul 5, 2017

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Jun 23, 2017

The task_id in queues can be used for 2 purposes

  1. Create multiple tasks with the same task_id so that they can run one after the other
  2. Logging to help debugging a workflow (e.g. service provisioning)

In the places where the task_id is being used for tracking a collection of tasks, this PR changes it to tracking_label.

@mkanoor
Copy link
Contributor Author

mkanoor commented Jun 23, 2017

@Fryguy @kbrock
Please review

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

This will be great.

@Fryguy
Copy link
Member

Fryguy commented Jun 26, 2017

@mkanoor I'd like to review these with you just to ensure they are all actually just label purposes and not "cannot run concurrently" purposes.

@mkanoor
Copy link
Contributor Author

mkanoor commented Jun 26, 2017

@@ -62,7 +62,7 @@ def job_check_for_evm_snapshots(job_not_found_delay)

def job_proxy_dispatcher_dispatch
if JobProxyDispatcher.waiting?
queue_work_on_each_zone(:class_name => "JobProxyDispatcher", :method_name => "dispatch", :task_id => "job_dispatcher", :priority => MiqQueue::HIGH_PRIORITY, :role => "smartstate")
queue_work_on_each_zone(:class_name => "JobProxyDispatcher", :method_name => "dispatch", :tracking_label => "job_dispatcher", :priority => MiqQueue::HIGH_PRIORITY, :role => "smartstate")
Copy link
Member

Choose a reason for hiding this comment

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

I think this is intentional to avoid multiple job_proxy_dispatchers from running simultaneously.

queue_work(:class_name => "MiqServer", :method_name => "log_status", :task_id => "log_status", :server_guid => MiqServer.my_guid, :priority => MiqQueue::HIGH_PRIORITY)
queue_work(:class_name => "MiqWorker", :method_name => "log_status_all", :task_id => "log_status", :server_guid => MiqServer.my_guid, :priority => MiqQueue::HIGH_PRIORITY)
queue_work(:class_name => "MiqServer", :method_name => "log_status", :tracking_label => "log_status", :server_guid => MiqServer.my_guid, :priority => MiqQueue::HIGH_PRIORITY)
queue_work(:class_name => "MiqWorker", :method_name => "log_status_all", :tracking_label => "log_status", :server_guid => MiqServer.my_guid, :priority => MiqQueue::HIGH_PRIORITY)
Copy link
Member

Choose a reason for hiding this comment

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

I think this to avoid logging multiple times, but we can probably dump it altogether. Can't see why this would need a tracking label.

Copy link
Member

Choose a reason for hiding this comment

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

Also please keep the lines aligned as was previously.

:instance_id => id,
:method_name => "do_post_provision",
:deliver_on => 1.minutes.from_now.utc,
:tracking_label => "#{self.class.name.underscore}_#{id}",
Copy link
Member

Choose a reason for hiding this comment

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

Can this just be inherited, and thus not explicitly listed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fryguy I will do this in a separate PR

:args => [args],
:role => 'automate',
:zone => options.fetch(:miq_zone, zone),
:tracking_label => "#{self.class.name.underscore}_#{id}"
Copy link
Member

Choose a reason for hiding this comment

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

This seems duplicate to the "deliver_to_automate" above...can we extract and DRY up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fryguy I will do this in a separate PR

mkanoor added 4 commits July 5, 2017 14:40
In the places where the task_id is being used for tracking a collection
of tasks, change it to tracking_label.
This area is being reworked by Greg M and Tina
@mkanoor mkanoor force-pushed the change_to_use_tracking_label branch from b82fc70 to 5b4b1ea Compare July 5, 2017 18:44
@miq-bot
Copy link
Member

miq-bot commented Jul 5, 2017

Checked commits mkanoor/manageiq@5576c7e~...5b4b1ea with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 4 offenses detected

app/models/service_template_provision_task.rb

spec/models/service_reconfigure_task_spec.rb

spec/models/service_template_provision_task_spec.rb

@Fryguy Fryguy merged commit 41f95f0 into ManageIQ:master Jul 5, 2017
@Fryguy Fryguy added this to the Sprint 64 Ending Jul 10, 2017 milestone Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants