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.

Sign up for GitHub

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

Jump to bottom

Add MiqQueue#tracking_label #15224

Merged
merged 1 commit into from
Jun 23, 2017
Merged

Add MiqQueue#tracking_label #15224

merged 1 commit into from
Jun 23, 2017

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented May 24, 2017 •
edited
Loading

Many times we pass a task_id into MiqQueue.put for logging only. Especially in automate.

This is not the best use of task_id as it prevents other workers from working concurrently on these jobs. Also, it is not a mechanism that works across other implementations of queues, so we are trying to phase it out.

This gives us a transition path away from using task_id, while still keeping the benefits of logging.
This also allows us to transition away from the global variable holding the current MiqQueue record.
related to ManageIQ/manageiq-gems-pending#183
and ManageIQ/manageiq-schema#17

/cc @mkanoor

@kbrock kbrock requested a review from Fryguy May 24, 2017 22:21
@kbrock kbrock mentioned this pull request May 26, 2017
Merged
@kbrock kbrock force-pushed the miq_queue_logging_task_id branch from d9e21de to 5383163 Compare May 30, 2017 21:33
@kbrock kbrock changed the title Add MiqQueue#logging_task_id Add MiqQueue#request_uuid May 30, 2017
Fryguy
Fryguy reviewed May 30, 2017
View reviewed changes
db/migrate/20170524211742_add_logging_task_id_to_miq_queue.rb Outdated
@@ -0,0 +1,5 @@
class AddLoggingTaskIdToMiqQueue < ActiveRecord::Migration[5.0]
Copy link
Member

Choose a reason for hiding this comment

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

Should this class (and thus file) be named AddRequestUUIDTOMiqQueue?

@kbrock kbrock force-pushed the miq_queue_logging_task_id branch from 5383163 to d2dd0c9 Compare May 31, 2017 13:16
Fryguy
Fryguy reviewed Jun 12, 2017
View reviewed changes
app/models/miq_queue_worker_base/runner.rb Outdated
@@ -104,6 +104,7 @@ def deliver_queue_message(msg)

begin
$_miq_worker_current_msg = msg
Thread.current["request_id"] = msg.task_id || msg.request_uuid
Copy link
Member

Choose a reason for hiding this comment

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

Should we change MiqQueue.put to automatically default request_uuid to task_id? That way this code can just be

Thread.current["request_id"] = msg.request_uuid

@Fryguy
Copy link
Member

Fryguy commented Jun 12, 2017

Overall I am 👍 on this PR, I just don't like the names...

request in my mind implies a UI request/response cycle, and that's not what this is, so I feel the word request makes it confusing
uuid implies a GUID structure, but this can be any freeform string

I originally thought queue_task_id, but @bzwei pointed out some problems with that name, so I'm not sure.

@kbrock kbrock changed the title Add MiqQueue#request_uuid [WIP] Add MiqQueue#request_uuid Jun 13, 2017
@kbrock kbrock added the wip label Jun 13, 2017
@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2017

@kbrock I think you sent me a new name on Gitter this morning, but I can't find it...what was your suggestion? Something like work_unit_name?

@kbrock
Copy link
Member Author

kbrock commented Jun 13, 2017

@Fryguy work_unit_uuid (more rails friendly than _id)

@Fryguy
Copy link
Member

Fryguy commented Jun 16, 2017 •
edited
Loading

As I mentioned above I don't like the uuid in the name because

uuid implies a GUID structure, but this can be any freeform string

However, what about just dropping a suffix altogether and going with work_unit?

@kbrock
Copy link
Member Author

kbrock commented Jun 22, 2017

@Fryguy for reference, I was thinking something like https://github.com/theforeman/foreman/blob/develop/lib/middleware/tagged_logging.rb#L8

@mkanoor
Copy link
Contributor

mkanoor commented Jun 22, 2017

@kbrock @Fryguy
Can we call it tracking_id similar to UPS/FedEx tracking, it allows us to track the job as it flows thru the system

@miq-bot
Copy link
Member

miq-bot commented Jun 22, 2017

This pull request is not mergeable. Please rebase and repush.

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

@Fryguy
Copy link
Member

Fryguy commented Jun 22, 2017

@mkanoor Also, I like the tracking bit, but you can't have the _id as that implies a Rails foreign key (or rather, when it doesn't as in this case, it's confusing)

@mkanoor
Copy link
Contributor

mkanoor commented Jun 22, 2017

@Fryguy
How about tracking_label?

@kbrock kbrock mentioned this pull request Jun 22, 2017
Merged
@kbrock kbrock force-pushed the miq_queue_logging_task_id branch from d2dd0c9 to c73f419 Compare June 22, 2017 21:40
@kbrock kbrock changed the title [WIP] Add MiqQueue#request_uuid Add MiqQueue#tracking_label Jun 22, 2017
@mkanoor
Copy link
Contributor

mkanoor commented Jun 23, 2017

@kbrock
We would have to pass the tracking_label to the next queue entries, when one queued task ends up queueing another task, akin to the task_id logic here
https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_queue.rb#L111

@kbrock kbrock force-pushed the miq_queue_logging_task_id branch from c73f419 to e403d6f Compare June 23, 2017 14:03
@Fryguy
Copy link
Member

Fryguy commented Jun 23, 2017

ManageIQ/manageiq-schema#17 merged

@kbrock kbrock force-pushed the miq_queue_logging_task_id branch from e403d6f to baef0ff Compare June 23, 2017 15:17
bzwei
bzwei reviewed Jun 23, 2017
View reviewed changes
app/models/miq_queue.rb Outdated
@@ -109,6 +109,7 @@ def self.put(options)
options[:queue_name] ||= create_with_options[:queue_name] || "generic"
options[:msg_timeout] ||= create_with_options[:msg_timeout] || TIMEOUT
options[:task_id] = $_miq_worker_current_msg.try(:task_id) unless options.key?(:task_id)
options[:tracking_label] = Thread.current["tracking_label"] || options[:task_id] unless options.key?(:tracking_label)
Copy link
Contributor

Choose a reason for hiding this comment

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

why "tracking_label"? can it be a symbol?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bzwei huh. turns out both will do the same thing.
I can change it, but had already merged this for the vmdb-logger.

It looks like you can use symbols and strings interchangeably.

>Thread.current[:test] = "abc"
>puts Thread.current["test"]
abc
>Thread.current["test"] = "cde"
>puts Thread.current[:test]
cde

@kbrock
Add MiqQueue#tracking_label
d2ad01f 
many times we add a task_id for logging only.
As we are getting away from task_id,
we don't want to loose the logging.

This allows us to transition away from task_id
The tracking_label will be set in the target system,
so a single request can be traced through the various systems
@kbrock kbrock force-pushed the miq_queue_logging_task_id branch from baef0ff to d2ad01f Compare June 23, 2017 15:45
@miq-bot
Copy link
Member

miq-bot commented Jun 23, 2017

Checked commit kbrock@d2ad01f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@mkanoor
Copy link
Contributor

mkanoor commented Jun 23, 2017

@Fryguy please review

@Fryguy Fryguy merged commit bbd6bb6 into ManageIQ:master Jun 23, 2017
@Fryguy Fryguy added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 23, 2017
@kbrock kbrock deleted the miq_queue_logging_task_id branch June 24, 2017 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzwei bzwei bzwei left review comments

@Fryguy Fryguy Fryguy left review comments

Assignees

@Fryguy Fryguy

Labels
core/queue enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kbrock @Fryguy @mkanoor @miq-bot @bzwei