1084 prep template changes #1203
Conversation
…Data is attached to it. Adding a test for it too
There was a problem hiding this comment.
As a study can have several prep templates, this could cause collisions, could you change? Perhaps study.id + some random string.
There was a problem hiding this comment.
These are just the queues though. Given that tornado is single threaded, is it possible for this block of code to be executed twice without the first queue draining?
There was a problem hiding this comment.
Good point but this will not be the case if we allow for multi-threaded tornado or if we move this section to run as a separate IPython job. Better safe than sorry, no?
There was a problem hiding this comment.
What would the benefit be of using a thread pool vs. just running multiple web servers? Threading adds a lot of complexity, and I doubt qiita is thread-safe anyway. IPython jobs do not have shared memory, so I do not understand your concern there. Doing something random because it "seems" safer does not inherently make it safer.
There was a problem hiding this comment.
If you do not feel that is important and @josenavas thinks is fine, let's leave it as is ...
There was a problem hiding this comment.
It isn't a feeling as there is a logical conclusion here, right?
There was a problem hiding this comment.
Agree that is not likely to happen, however it is really easy to make it unique, so fixing because I think it can be a pain to try to debug an issue with this:
queue_name = "CREATE_PREP_TEMPLATE_%d_%d" % (study.id, id(md_template))
|
A couple of comments. |
|
Thanks @antgonza , addressed your comments. One more reviewer so this can get merged? |
|
👍 |
1 similar comment
|
👍 |
1084 prep template changes
4 participants
Built on top of #1202 (review/merge that one first).
This PR modifies the prep template object to reflect the changes in the DB. It also fixes the tests for the prep template object.
All the other tests expected to fail.