-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: mysql support #115
fix: mysql support #115
Conversation
0dc017f
to
66dd82a
Compare
@@ -54,7 +54,7 @@ jobs: | |||
|
|||
- name: 'Run Integration Tests' | |||
run: | | |||
./gradlew --no-parallel --no-daemon test-all-environments --info | |||
./gradlew --no-parallel --no-daemon test-mysql-aurora --info |
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.
Temporary change to run integration tests
0863fd1
to
10a864a
Compare
d9a3ff4
to
0827b5b
Compare
ed2b4ca
to
d6aa0b0
Compare
] | ||
|
||
def is_network_exception(self, error: Optional[Exception] = None, sql_state: Optional[str] = None) -> bool: | ||
if isinstance(error, QueryTimeoutError): |
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.
Does MySQL have a ConnectionTimeoutError that we should check for like PG? Or is the connection timeout caught in the logic below?
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.
@@ -55,6 +55,16 @@ class FailoverPlugin(Plugin): | |||
"force_connect", | |||
"notify_host_list_changed"} | |||
|
|||
_METHODS_REQUIRE_UPDATED_TOPOLOGY: Set[str] = { | |||
"Connection.commit", |
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.
how did we come up with this method list, and why is it necessary?
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.
Mysql specific thing awslabs/aws-mysql-jdbc#364
aws_wrapper/failover_plugin.py
Outdated
@@ -408,6 +419,9 @@ def _allowed_on_closed_connection(method_name: str): | |||
method_name == "Connection.getSchema" or \ | |||
method_name == "Connection.getTransactionIsolation" | |||
|
|||
def _can_update_topology(self, method_name: str): |
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.
would _requires_topology_update
be a more accurate name? to me can_update_topology makes it sound like the methods listed above (commit, rollback etc) can cause a topology update in and of themselves
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.
Makes sense
0827b5b
to
841fcd8
Compare
f3aab9f
to
301407a
Compare
841fcd8
to
6907ba0
Compare
301407a
to
30c9fcc
Compare
30c9fcc
to
a00c45d
Compare
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.