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

Update old MySQL packages #1364

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Update old MySQL packages #1364

wants to merge 12 commits into from

Conversation

rzats
Copy link
Contributor

@rzats rzats commented Jan 4, 2024

Closes #1355.

Summary:

Bumps versions for the mysqlclient and SQLAlchemy libraries.

Removes the mysql-connector and pymysql libraries, replacing all their usage with the MySQLdb interface.

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

@rzats rzats requested a review from melange396 January 4, 2024 16:00
@melange396
Copy link
Collaborator

did you see the comments i added to #1355 yesterday?

Copy link
Collaborator

@melange396 melange396 left a 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.

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.

integrations/server/test_covidcast_endpoints.py Outdated Show resolved Hide resolved
src/acquisition/covidcast/database.py Show resolved Hide resolved
src/acquisition/fluview/fluview_update.py Outdated Show resolved Hide resolved
requirements.api.txt Show resolved Hide resolved
@rzats
Copy link
Contributor Author

rzats commented Jan 18, 2024

@melange396 the rowcount property is available in the new cursor API, so no issues there :)

As for a quick performance check, let me try:

/run performance test

Copy link
Contributor

✅ Performance tests complete! Result summary:

  • Total requests: 1000
  • Total failures: 12
  • Min response time: 7.69 ms
  • Max response time: 128314.30 ms
  • Average response time: 1334.67 ms
  • Median response time: 380.00 ms
  • Requests per second: 3.17

Click here to view full results: https://github.com/cmu-delphi/delphi-epidata/actions/runs/7572075216.

@melange396
Copy link
Collaborator

As for a quick performance check, let me try:

i dont think that demonstrates very much, since its not doing acquisition or even going through the integration tests

Copy link

sonarcloud bot commented Jan 23, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
8.1% Duplication on New Code

See analysis details on SonarCloud

@melange396
Copy link
Collaborator

Do you have any updates on performance comparison and/or compatibility with the old long INSERT INTO...UPDATE statement?

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.

Update database drivers
2 participants