Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support high-availability setups for the interactive tools proxy #18481
Support high-availability setups for the interactive tools proxy #18481
Changes from 3 commits
b54bf35
bf6b6ed
967b591
d85e1b5
ce6d0f7
647fd4c
678bea1
dddf5ba
6be9b60
e7dbe42
c1fd2b5
b6ad695
5e8ca55
26fd210
ffa8ea4
018e541
e040750
8305016
6fc0d46
4e6688c
0b6ad17
9ee6636
5c0b5d6
7510372
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I may be missing something regarding the
FileLock
. What role was it actually playing? Isn't SQLite already taking care of locks?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Galaxy has a long history of concurrency issues with respect to SQLite. I cannot promise this FileLock was needed but experience taught me it was better to be safe than sorry here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some information that may help us understand whether the lock is needed in this case.
From both excerpts I understand that it is likely that the long history of concurrency issues that Galaxy has with SQLite probably has to do with using the SQLAlchemy ORM under high load (maybe you can either support/refute this?).
As far as I know, the interactive tools mapping file is written only by
InteractiveToolSqlite
(orInteractiveToolPropagatorSQLAlchemy
in this refactor) and read only by Galaxy (the same class) and the proxy. I understand that issues would appear if interactive tools are in extremely high demand (Galaxy constantly writes keys and tokens to the file). However, in that case, wouldn't it just make better sense, as the SQLite docs claim, that the Galaxy admin uses PostgreSQL rather than SQLite? Another solution would be increasing the SQLite lock timeout.Maybe if @jdavcs agrees to go forward with a new config property for mappings based on arbitrary SQLAlchemy database URLs, it may even make sense to discourage the use of the original property
interactivetools_map
in the Galaxy docs to favour the use of Postgres? After all, SQLite use is already discouraged.In that case though, I think we should also have a look together at the proxy-side implementation, because it also has it's drawbacks regarding database setup (maybe it should just be done automatically by Galaxy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the core of the issue here is just SQLite: "SQLite will still always lock the database file once a transaction is started" - it's just suboptimal for write-intensive scenarios. Using the ORM makes it worse: the autoflush model mentioned in the docs ensures that the state of objects loaded from the db is kept in sync with the data in the db by issuing DML statements before retrieving new data (when needed) - so the db file gets locked much more frequently if we rely on the Session. However, we don't use the ORM here.
If we go with this proposed approached, we can still keep SQLite as an option (it may be still a good enough option for smaller instances or dev purposes, etc.; after all we support SQLite as an option for the main database too). We may consider adding a note to Galaxy's documentation and updating the IT tutorial accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note (dddf5ba) to the documentation and opened galaxyproject/training-material#5178 and galaxyproject/training-material#5179