Project

Profile

Help

Task #2318

Ensure thread-safety of ProgressReport models

Added by bmbouter about 3 years ago. Updated 6 months ago.

Status:
MODIFIED
Priority:
Normal
Category:
-
Sprint/Milestone:
Start date:
Due date:
% Done:

100%

Platform Release:
Blocks Release:
Backwards Incompatible:
No
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Documentation
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:
Sprint 11

Description

Django models changes can be thread safe0, but some care needs to be taken. We need the Progressreport models specifically to be thread safe.

This likely involves 2 things:

1. rewriting the implementaiton to be thread safe
2. Using documentation which identifies the thread-safety concern for plugin writers updating models themselves from many threads.

[0]: http://stackoverflow.com/questions/26234819/are-django-models-thread-safe#26234946


Checklist

Associated revisions

Revision ee3ec288 View on GitHub
Added by dkliban@redhat.com almost 3 years ago

Adds docs about using ProgressBar with threads

closes #2318
https://pulp.plan.io/issues/2318

Revision ee3ec288 View on GitHub
Added by dkliban@redhat.com almost 3 years ago

Adds docs about using ProgressBar with threads

closes #2318
https://pulp.plan.io/issues/2318

Revision ee3ec288 View on GitHub
Added by dkliban@redhat.com almost 3 years ago

Adds docs about using ProgressBar with threads

closes #2318
https://pulp.plan.io/issues/2318

History

#1 Updated by mhrivnak about 3 years ago

It is likely just the incrementing feature that needs to be thread-safe. The pattern will probably be:

  • create a progress object
  • create a Queue or similar
  • create some threads and give them a reference to the queue, and probably the incrementor that comes with the progress object
  • each thread runs in a loop: grab a job from the queue, do it, call queue.task_done(), and increment the progress object
  • the main thread calls queue.join() and waits for the work to finish

It's worth considering if there's a way to either wrap or somehow hook into the queue's task_done method, but that's optional. There may not be a reasonable way to do so.

#2 Updated by bmbouter about 3 years ago

Yes item 1 is likely as simple as the increment() method only being updated. I think most of the work is in the docs.

#3 Updated by mhrivnak about 3 years ago

  • Groomed changed from No to Yes

#4 Updated by mhrivnak almost 3 years ago

  • Sprint/Milestone set to 29

#5 Updated by dkliban@redhat.com almost 3 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to dkliban@redhat.com

#6 Updated by dkliban@redhat.com almost 3 years ago

https://github.com/pulp/pulp/pull/2871#issuecomment-264014822

Based on the conversation on the above PR, it sounds like we only need to add some documentation that states that a single instance of a ProgressBar or ProgressSpinner objects should be used by all threads when processing items in parallel.

#7 Updated by bmbouter almost 3 years ago

  • Checklist item add docstring to ProgressBar added
  • Description updated (diff)
  • Tags Documentation added

After some lunchtime conversation, it was confirmed that this is a documentation only change and that the docstrings should recommend the user have all threads share the same instance of ProgressBar. I don't think ProgressSpinner and ProgressReport need updates since updating those frequently will almost never occur.

I'm setting the documentation tag and rewriting the story some.

#8 Updated by dkliban@redhat.com almost 3 years ago

  • Status changed from ASSIGNED to POST

#9 Updated by dkliban@redhat.com almost 3 years ago

  • Checklist item add docstring to ProgressBar set to Done

#10 Updated by dkliban@redhat.com almost 3 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#11 Updated by bmbouter over 1 year ago

  • Sprint set to Sprint 11

#12 Updated by bmbouter over 1 year ago

  • Sprint/Milestone deleted (29)

#13 Updated by daviddavis 6 months ago

  • Sprint/Milestone set to 3.0

#14 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3)

Please register to edit this issue

Also available in: Atom PDF