-
Notifications
You must be signed in to change notification settings - Fork 68
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 old MySQL packages #1364
base: dev
Are you sure you want to change the base?
Conversation
did you see the comments i added to #1355 yesterday? |
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.
Nice, this is pretty much a drop-in replacement except for the cursor name accessor. Does this cursor.rowcount
still work with the new adapter? If the method exists, i would assume so, but you never know.
delphi-epidata/src/acquisition/covidcast/database.py
Lines 175 to 181 in 8a476fd
modified_row_count = self._cursor.rowcount | |
self._cursor.execute(fix_is_latest_issue_sql) | |
if not suppress_jobs: | |
self.run_dbjobs() # TODO: incorporate the logic of dbjobs() into this method [once calls to dbjobs() are no longer needed for migrations] | |
if modified_row_count is None or modified_row_count == -1: | |
# the SQL connector does not support returning number of rows affected (see PEP 249) |
We talked about performance differences between the adapters -- i expect this to be approximately the same as with the previous one if not a teeny bit better. But just in case that doesnt hold, it would be good to try timing some stress-testing. From a simpler perspective, how does the running time of the integration tests compare to before?
If you have time, could you try to see if this adapter would support the reeeeeally long INSERT INTO...UPDATE statement that broke things before #1356? I have a strong feeling that the old "upsert" is better for data file fragmentation than the new REPLACE INTO.
@melange396 the As for a quick performance check, let me try:
|
✅ Performance tests complete! Result summary:
Click here to view full results: https://github.com/cmu-delphi/delphi-epidata/actions/runs/7572075216. |
i dont think that demonstrates very much, since its not doing acquisition or even going through the integration tests |
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Do you have any updates on performance comparison and/or compatibility with the old long INSERT INTO...UPDATE statement? |
Closes #1355.
Summary:
Bumps versions for the
mysqlclient
andSQLAlchemy
libraries.Removes the
mysql-connector
andpymysql
libraries, replacing all their usage with theMySQLdb
interface.Prerequisites:
dev
branchdev