-
Notifications
You must be signed in to change notification settings - Fork 41
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
failover2 on connect #1133
failover2 on connect #1133
Conversation
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2023.3.1
with:
upload-result: true Contact Qodana teamContact us at qodana-support@jetbrains.com
|
wrapper/src/main/java/software/amazon/jdbc/plugin/failover2/FailoverConnectionPlugin.java
Show resolved
Hide resolved
boolean isInitialConnection, JdbcCallable<Connection, SQLException> connectFunc, boolean isForceConnect) | ||
throws SQLException { | ||
|
||
if (props.containsKey(INTERNAL_CONNECT_PROPERTY_NAME)) { |
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 it work to change this condition to the following so that we don't have to add the INTERNAL_CONNECT_PROPERTY_NAME? I think calling forceConnect probably always indicates we just want to connect directly without any special additional logic. If this works, we can probably just get rid of connectInternal and have different implementations for connect vs forceConnect.
if (props.containsKey(INTERNAL_CONNECT_PROPERTY_NAME)) { | |
if (isForceConnect)) { |
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.
forceConnect()
is for direct connection but it's not used in failover2
. So forceConnect()
makes not much sense to this case. Also, failover2
uses regular connect()
because MonitoringHostListProvider
already verified topology so no need to force direct connect. And with a regular connect()
, the call comes to failover2
plugin again and our intention is to skip it.
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 see. Using the INTERNAL_CONNECT_PROPERTY_NAME property as a flag seems a bit hacky/strange to me but I can't think of an alternative at the moment.
wrapper/src/main/java/software/amazon/jdbc/plugin/failover2/FailoverConnectionPlugin.java
Outdated
Show resolved
Hide resolved
3b0e0fd
to
b84d66c
Compare
throws SQLException { | ||
|
||
if (!ENABLE_CONNECT_FAILOVER.getBoolean(props)) { | ||
return connectFunc.call(); |
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.
Shouldn't we be connecting using getVerifiedConnection in this case?
return connectFunc.call(); | |
return this.staleDnsHelper.getVerifiedConnection(isInitialConnection, this.hostListProviderService, | |
driverProtocol, hostSpec, props, connectFunc); |
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.
getVerifiedConnection() call already been made of a caller of connect() that is the same failover2 plugin. I don't think we need to execute it again.
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.
sorry are you saying that getVerifiedConnection has been previously called by another plugin or previously called by failover2?
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.
failover2 connect()
is called, then getVerifiedConnection()
is called.
Line 654 in b84d66c
conn = this.staleDnsHelper.getVerifiedConnection(isInitialConnection, this.hostListProviderService, |
After that, if connection isn't established, a failover starts.
Line 664 in b84d66c
this.failover(hostSpec); |
And eventually it calls pluginService.connect()
Line 507 in b84d66c
writerCandidateConn = this.pluginService.connect(writerCandidate, copyProp); |
that will call failover2 connect()
again. This call can be passed through to the next plugin since it's already been handled earlier.
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.
If I understand correctly that explains why we should use connectFunc.call instead of getVerifiedConnection when the props contain the INTERNAL_CONNECT_PROPERTY_NAME property, but this particular line is covering a different scenario: it is hit when connect failover is disabled. When connect failover is disabled we will never enter the failover logic below, so as the code currently is written, getVerifiedConnection will never be called in this scenario
b84d66c
to
df3d833
Compare
Summary
Allows failover2 plugin to perform a failover during opening a new connection.
Additional Reviewers
@karenc-bq
@aaron-congo
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.