URGENT: Fix util #1324

josenavas · 2015-07-03T03:05:32Z

This PR fixes the util.py file to use transactions.
Since some function signatures changed, I had to fix other parts of the code to make the tests to pass.
Any future PR will be based on this one, hence that is why it is urgent (the other ones will be in parallel, but they'll rely on the changes in util.py).

Some comments:

purge_filepaths is tricky. It is not used anywhere but it has the potential of leaving the DB out of sync with the FS if it is executed inside a bigger transaction. I haven't put any check on it, but one option will be to make it use it's own transaction. Since each Transaction object has it's own connection, this will not be an issue. However, I'd like to hear what others think.
get_preprocessed_params_tables was missing tests and it was incorrect.
find_repeated was not used anywhere, I removed it as we can use skbio's find_duplicates

antgonza · 2015-07-03T13:18:54Z

qiita_db/util.py

+                     move_files=True):
+    r"""Inserts `filepaths` in the database.
+
+    Since the files live outside the database, the directory in which the files


mvoes -> moves

antgonza · 2015-07-03T13:25:36Z

Minor comments. Now, what about extending the idea of transactions so we only execute code once we know the transaction is done. For example, if something fails and you are using purge_filepaths do not move them until the full transaction is done. This could also be helpful for patches. Also, the codes looks fine but I think the biggest worry is not about the changes you made but about the possible missing lines that needed changing. Is there a way to verify that things that calls to any function that has SQL are actually being run in transaction?

josenavas · 2015-07-03T16:18:47Z

Thanks @antgonza. Once everything is changed, a quick search in the code base for SQLConnectionHandler will tell us if there is anything executed outside a transaction.

I agree with you re executing some code once the transaction is done (e.g. moving files). This is a tricky question and it really depends on the operation performed.

For example, in purge_filepaths, if the transaction successfully completes and there is a problem removing files, it is not a big deal, as the only "bad" thing is that the file system is dirty (which is easy to write a function for cleaning it up in a cron job).

However, in move_filepaths_to_uploads_folder, the function is usually executed when unlinking files from raw data. If there is a problem moving files but the transaction is committed, those files are "lost" from the user point of view: they live under the raw data folder, but we've lost the filename and the user cannot access them. In this case, you want to rollback the operation.

The only idea that comes to my mind is this:
We add a list of tuples to the Transaction object, where each tuple is of the form [(src_path, dest_path)]. During the code we move the files w/o a problem, but we add them to this list. If the transaction at some point performs a rollback, it is in charge of moving back those files, so the DB and the filesystem are in sync.
Also, we add 2 list of strings, which will be "paths to remove on commit" and "paths to remove on rollback". In the former, the transaction will remove those files if it commits (e.g. for purge_filepaths) and in the later it will remove those files in rollback (e.g. creating a qiime_mapping_file).This way, we can make sure that all the filepaths that are in the DB are present in the filesystem, while we are allowing the filesystem to have files that they might not be in the DB (which I think is not a big deal).

What do you think @antgonza, is this a good solution?

ElDeveloper · 2015-07-03T23:00:27Z

qiita_db/util.py

+            # alrady exists on the DB
+            db_path = partial(join, base_fp)
+            new_filepaths = [
+                (db_path("%s_%s" % (obj_id, basename(path))), id)


We usually avoid id as a var name, but I see this was like this in the original version of the code.

Good point, done.

ElDeveloper · 2015-07-03T23:26:06Z

Looks good @josenavas, just a few tiny comments and a question.

josenavas · 2015-07-04T00:41:37Z

Thanks @ElDeveloper and @antgonza

I've added some functionality to the Transaction object so we can make sure that functions like purge_filepaths do not break the DB.

ElDeveloper · 2015-07-04T01:29:01Z

qiita_db/sql_connection.py

+        self._post_commit_funcs = []
+        self._post_rollback_funcs = []
+        if error_msg:
+            raise RuntimeError(


I can't find a test that checks for a case where 💩 hits the fan, can you add one?

josenavas · 2015-07-04T02:38:19Z

Thanks @ElDeveloper

ElDeveloper · 2015-07-04T02:46:47Z

qiita_db/sql_connection.py

@@ -1012,6 +1014,22 @@ def execute_fetchindex(self, idx=-1):
        """
        return self.execute()[idx]

+    def _funcs_executor(self, cmds, func_str):


Thanks, I think you still missed changing cmds here and in the tests.

Good call. I think I've fixed them all now...

squirrelo · 2015-07-04T03:03:42Z

The _clean_up function seems to be missing the func calls for post-rollback and post-commit (line 750).

josenavas · 2015-07-04T03:05:18Z

Those calls are inside the commit and rollback functions

squirrelo · 2015-07-04T03:06:50Z

Ah, ok. I get the centralization logic here.

squirrelo · 2015-07-04T03:10:23Z

qiita_db/sql_connection.py

+        error_msg = []
+        for cmd in cmds:
+            try:
+                cmd()


Will these function calls ever need parameters passed? If so, we should have a way to do this (kwargs or whatever is easiest).

Please read the documentation. There are ways or making this viable always, check the purge_filepath function.

antgonza · 2015-07-04T13:32:45Z

I think this looks ready ... 👍

josenavas · 2015-07-04T19:31:45Z

Ready for another review.
I've created the function exectue_fetchflatten so we can centralize in a single function the loop that we were doing in multiple places.

Given that @squirrelo felt strong about it, I end up fixing #1325. However, I did not implment @squirrelo proposal. His proposal was hackish and not standard, so I've actually implemented a standard way of doing this.

ElDeveloper · 2015-07-04T21:07:43Z

👍

URGENT: Fix util

josenavas added 4 commits July 2, 2015 18:31

Fixing util

62d2e9c

Fixing calls to util functions that changed the API

c2a4293

Fixing small bug

a6a176c

Fixing error in get_lat_longs

1d3c0fe

josenavas added the priority: high label Jul 3, 2015

josenavas added this to the Alpha 0.2 milestone Jul 3, 2015

josenavas mentioned this pull request Jul 3, 2015

WIP: Transaction #1318

Merged

28 tasks

antgonza reviewed Jul 3, 2015
View reviewed changes

Addressing comments

2e04a1d

ElDeveloper reviewed Jul 3, 2015
View reviewed changes

josenavas added 3 commits July 3, 2015 17:21

Adding post commit/rollback functionality

52d65a7

Addressing comments

7a4aedd

Ensuring FS-DB sync with post commit/rollback funcs

0bc8638

ElDeveloper reviewed Jul 4, 2015
View reviewed changes

Addressing comments

90b985d

Adding comment

ad9f87e

ElDeveloper reviewed Jul 4, 2015
View reviewed changes

squirrelo reviewed Jul 4, 2015
View reviewed changes

josenavas added 4 commits July 4, 2015 11:46

Adding execute_flatten function

c0c72ff

Make use of fetchflatten

49c302f

Improving add function methods

703c01f

Removing unused import

8d18255

ElDeveloper added a commit that referenced this pull request Jul 4, 2015

Merge pull request #1324 from josenavas/pr-1318-util

e4764b2

URGENT: Fix util

ElDeveloper merged commit e4764b2 into qiita-spots:transaction Jul 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

URGENT: Fix util #1324

URGENT: Fix util #1324

josenavas commented Jul 3, 2015

antgonza Jul 3, 2015

josenavas Jul 3, 2015

antgonza commented Jul 3, 2015

josenavas commented Jul 3, 2015

ElDeveloper Jul 3, 2015

josenavas Jul 4, 2015

ElDeveloper commented Jul 3, 2015

josenavas commented Jul 4, 2015

ElDeveloper Jul 4, 2015

josenavas Jul 4, 2015

josenavas commented Jul 4, 2015

ElDeveloper Jul 4, 2015

josenavas Jul 4, 2015

squirrelo commented Jul 4, 2015

josenavas commented Jul 4, 2015

squirrelo commented Jul 4, 2015

squirrelo Jul 4, 2015

josenavas Jul 4, 2015

antgonza commented Jul 4, 2015

josenavas commented Jul 4, 2015

ElDeveloper commented Jul 4, 2015

URGENT: Fix util #1324

URGENT: Fix util #1324

Conversation

josenavas commented Jul 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgonza commented Jul 3, 2015

josenavas commented Jul 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElDeveloper commented Jul 3, 2015

josenavas commented Jul 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josenavas commented Jul 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squirrelo commented Jul 4, 2015

josenavas commented Jul 4, 2015

squirrelo commented Jul 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgonza commented Jul 4, 2015

josenavas commented Jul 4, 2015

ElDeveloper commented Jul 4, 2015