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

Transactionaize patches and environment mgr #1337

Merged

Conversation

josenavas
Copy link
Contributor

Modifies the patches and the environment manager to use transactions.

Few notes:

  • the environment manager is tricky. There are a couple of places in which the SQLConnectionHandler should still be used because it is connecting to the DB with admin privileges, and is changing the connection type to set autocommit = True. Since autocommit is true, there is no point of having the transaction so using the old conn handler there. Also the Transaction does not support admin access.
  • in qiita_pet/test/tornado_test_base.py there was a bit of code duplication. I removed that code and used the function from qiita_db.

@josenavas josenavas added this to the Alpha 0.2 milestone Jul 7, 2015
@josenavas josenavas mentioned this pull request Jul 7, 2015
28 tasks
@josenavas
Copy link
Contributor Author

@antgonza @squirrelo this is ready for review.

'SELECT f.* from qiita.filepath f JOIN qiita.analysis_filepath afp ON '
'f.filepath_id = afp.filepath_id')
# retrieve relative filepaths as dictionary for matching
mountpoints = {m[1].rstrip('/\\'): m[0] for m in get_mountpoint(
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@antgonza
Copy link
Member

antgonza commented Jul 7, 2015

👍 if tests pass

@josenavas
Copy link
Contributor Author

errors were with my friend flake8

@@ -101,18 +103,21 @@ def _add_ontology_data(conn):
raise IOError("Error: Could not fetch ontologies file from %s" %
Copy link
Contributor

Choose a reason for hiding this comment

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

This part should probably also be in the TRN, since it can fail and cause rollback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed. If this is the first function, the transaction didn't even started. If another function inside a transaction is calling this, that other context will catch it and rollback.

@squirrelo
Copy link
Contributor

few comments.

@josenavas
Copy link
Contributor Author

@squirrelo comments answered

@squirrelo
Copy link
Contributor

👍

squirrelo added a commit that referenced this pull request Jul 7, 2015
Transactionaize patches and environment mgr
@squirrelo squirrelo merged commit a4773e1 into qiita-spots:transaction Jul 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants