Delete process data #1034
Delete process data #1034
Conversation
| conn_handler.add_to_queue(queue, sql) | ||
|
|
||
| conn_handler.execute_queue(queue) | ||
|
|
There was a problem hiding this comment.
This also needs to delete the files on the filesystem, correct?
There was a problem hiding this comment.
This is something that we have not discussed as a group, but I will just make a note here. If you take a look, it doesn't even remove the row from qiita.filepath table. We are planning to run "purge_filepaths" in a cron job. That function, identifies which rows from qiita.filepath are not referenced and removes them from the table and from the filesystem. The advantage of this, is that the server is more free to answer requests rather than doing maintenance work, that can be done by a worker.
There was a problem hiding this comment.
For both comments: What we are currently doing is to leave the filepath entry (it doesn't affect anything) as deleting files could be pretty expensive. Additionally, the plan is to drop one of the already created functions once we have this in: #814
There was a problem hiding this comment.
OK, can an issue be opened specifically for that then?
There was a problem hiding this comment.
BTW @josenavas, we discussed this decision as a group, search in your email for "deleting files and automatic jobs"
There was a problem hiding this comment.
My question then is will that job be fired off as soon as the server starts or will there be a delay? Any delay means that the files could sit around indefinitely, as the user could start the server, quickly do something, and then shut it down again without that cleanup ever firing.
Also I do not have that email in any of my accounts.
There was a problem hiding this comment.
Sorry about that @antgonza I found the email. We should write those small parts on the Contributing.md file. Next week, I'm going to put a new version together and I will submit a PR, so we can all review it.
The reason why I thought it was not discussed is because that is handled inconsistently across the code base, so I thought that was not agreed. Thanks for pointing that out!
@squirrelo I forwarded the email to you for your reference.
There was a problem hiding this comment.
- Will you mind moving the discussion about how the job is gonna work
to the actual issue? That way we will ensure everybody's comments are
taken into account when that's resolved. Thanks! - Thread to which you replied:
https://groups.google.com/forum/#!searchin/qiita-dev/deleting$20files$20and$20automatic$20jobs/qiita-dev/X8pu1fHASuY/TT2_fARyS50J
There was a problem hiding this comment.
Sure. Will just conclude by saying if this pull request is leveraging that expected functionality, that functionality should probably go in pretty quickly after this request.
|
Few comments. Will test interface once addressed. |
|
Ready for another review. |
|
|
||
| if analysis_ids: | ||
| raise QiitaDBError( | ||
| "Processed data %d is linked to (meta)analyses: %s" % |
There was a problem hiding this comment.
Can we improve the error message here?
Processed data %d cannot be removed because it is linked to the following (meta)analysis: %s
There was a problem hiding this comment.
analysis names would be better than IDs. There is no way to link an ID to an analysis in the UI
There was a problem hiding this comment.
Alternately, raise error without the IDs and use the Logger to store the IDs
There was a problem hiding this comment.
Going with adding names and QiitaDBError so the user can see the error.
There was a problem hiding this comment.
One thing I just thought of if showing names: this will potentially show names of analyses that the user does not have access to. Probably not the end of the world, but could be a privacy issue.
There was a problem hiding this comment.
There was a problem hiding this comment.
And users should not be able to put it back to sandbox if it was public and
there was analysis using it.
2015-04-03 9:31 GMT-07:00 Antonio Gonzalez notifications@github.com:
In qiita_db/data.py
#1034 (comment):
If the processed data status is not sandbox"""if cls(processed_data_id).status != 'sandbox':raise QiitaDBStatusError("Illegal operation on non sandbox processed data")
conn_handler = SQLConnectionHandler()
analysis_ids = [str(_id[0]) for _id in conn_handler.execute_fetchall("SELECT DISTINCT analysis_id FROM qiita.analysis_sample WHERE ""processed_data_id = {0} ORDER BY ""analysis_id".format(processed_data_id))]
if analysis_ids:raise QiitaDBError("Processed data %d is linked to (meta)analyses: %s" %I think that's fine. The only way this could happen is if the owner made
it public, then move it to sandbox, and is trying to delete it. Which will
be a full other beast ...—
Reply to this email directly or view it on GitHub
https://github.com/biocore/qiita/pull/1034/files#r27738714.
Jose Navas
|
Few more comments @antgonza. Thanks for putting this together! |
| QiitaDBError | ||
| If the processed data has (meta)analyses | ||
| """ | ||
| if cls(processed_data_id).status != 'sandbox': |
There was a problem hiding this comment.
I'm just thinking about this. Should we limit the removal only to sandbox? I think we should be able to remove the processed data in any moment except if it is public (i.e. why we should not allow the removal if it is private?)
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, fine with me. We can revisit this later and it is an easy change if we decide something different. I'm going to pull down the code to test.
|
👍 |
Adding Processed data deletion, both to the qiita_db and the GUI.