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

Update indexing logic to handle for unknown file_import tasks #2956

Merged

Conversation

scottx611x
Copy link
Member

@scottx611x scottx611x commented Aug 28, 2018

We should do something more elegant like utilizing celery's available signal: task_published signal, updating the state of published tasks to something we decide (SENT, PUBLISHED etc.) which would then allow us to treat the PENDING state that celery is assigning as a true "unknown" state. This would also lessen the djcelery package's grip on us.

To be able to achieve something like this, we first have to get our existing file import tasks in a good state (no pun intended) and properly categorized (Removal of false PENDINGS).

Note: these changes warrant another run of: ./manage.py update_index

Copy link
Member

@hackdna hackdna left a comment

Choose a reason for hiding this comment

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

This has worked since April (https://github.com/refinery-platform/refinery-platform/pull/2723/files). What is the actual bug and its root cause?

@scottx611x
Copy link
Member Author

scottx611x commented Aug 28, 2018

@hackdna It hasn't properly worked since the that TaskMeta portion in the search_indexes code was removed. We just hadn't noticed the side effects until this morning.

The actual bug is due to the fact that celery will report a PENDING state for tasks (import_file in this case) that are either truly PENDING or in an unknown state. @mccalluc noticed some Nodes that have been in the improper half of this PENDING state for awhile this morning.

I mention a better fix here, but that would still be dependent on this workaround getting applied first and getting our index back in the proper state.

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #2956 into release-1.6.6 will decrease coverage by 0.42%.
The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-1.6.6    #2956      +/-   ##
=================================================
- Coverage          59.69%   59.27%   -0.43%     
=================================================
  Files                433      433              
  Lines              27844    27146     -698     
  Branches            1274     1274              
=================================================
- Hits               16622    16091     -531     
+ Misses             11222    11055     -167
Impacted Files Coverage Δ
refinery/data_set_manager/tests.py 99.28% <ø> (ø) ⬆️
refinery/data_set_manager/search_indexes.py 94.44% <100%> (+2.37%) ⬆️
refinery/core/views.py 40.61% <0%> (-2.33%) ⬇️
refinery/data_set_manager/utils.py 74.95% <0%> (-2.11%) ⬇️
refinery/file_store/models.py 83.06% <0%> (-1.06%) ⬇️
refinery/core/api.py 38.43% <0%> (-0.46%) ⬇️
refinery/core/test_views.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6b5665...d1b9f9b. Read the comment docs.

@hackdna
Copy link
Member

hackdna commented Aug 28, 2018

It is still not clear to me what the bug is. What are the steps to reproduce, what is expected and what is actually happening?

@scottx611x
Copy link
Member Author

scottx611x commented Aug 28, 2018

The bug:
Currently, without the TaskMeta check, the index will be improperly updated with PENDING for someFileStoreItems when it should be N/A.

Observed:
https://test.stemcellcommons.org/data_sets/f27ce499-e6d7-494b-8190-48856c00be5c/#/files/

Celery will forget about task results after one day by default. Running an AsyncResult() after 24hrs will yield a PENDING state.

>>> from core.models import DataSet
>>> d = DataSet.objects.get(uuid="f27ce499-e6d7-494b-8190-48856c00be5c")
>>> [f.get_import_status() for f in d.get_file_store_items()]
['PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING']
>>> d = DataSet.objects.get(uuid="7d51c027-e529-4c38-a8fb-8f1d46aad009")
>>> [f.get_import_status() for f in d.get_file_store_items()]
['PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING', 'PENDING']

We need some way to determine whether these PENDING states that are reported actually correspond to a known task or not.

Expected:
All of the "importing" ("PENDING") files from https://test.stemcellcommons.org/data_sets/f27ce499-e6d7-494b-8190-48856c00be5c/#/files/ (and others) should actually have lightning bolts ("N/A") since they were never imported successfully.

@scottx611x
Copy link
Member Author

@hackdna Any thoughts on this?

@hackdna
Copy link
Member

hackdna commented Aug 29, 2018

Thanks for a detailed description. The problem is that even TaskMeta records are only kept for a month (not to mention that they won't even exist at all after django-celery is removed), so a different solution is needed.

@scottx611x scottx611x changed the title Reintroduce indexing logic to handle unknown file_import tasks Update indexing logic to handle for unknown file_import tasks Aug 29, 2018
Copy link
Member

@hackdna hackdna left a comment

Choose a reason for hiding this comment

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

Looks a lot better now. Approving with a caveat that _get_download_url_or_import_state() function and related functionality should be rewritten later.

@scottx611x scottx611x merged commit 07cb907 into release-1.6.6 Aug 29, 2018
@scottx611x scottx611x deleted the scottx611x/ensure_import_tasks_exist_during_index branch August 29, 2018 20:03
@scottx611x scottx611x added this to Backlog in Scott O. Tasks via automation Oct 24, 2018
@scottx611x scottx611x moved this from Backlog to Done in Scott O. Tasks Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants