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: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
| boolean isInitialConnection, JdbcCallable<Connection, SQLException> connectFunc, boolean isForceConnect) | ||
| throws SQLException { | ||
|
|
||
| if (props.containsKey(INTERNAL_CONNECT_PROPERTY_NAME)) { |
There was a problem hiding this comment.
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.
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.
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.
sergiyvamz
force-pushed
the
failover2-on-connect
branch
2 times, most recently
from
3b0e0fd to
b84d66c
Compare
| throws SQLException { | ||
|
|
||
| if (!ENABLE_CONNECT_FAILOVER.getBoolean(props)) { | ||
| return connectFunc.call(); |
There was a problem hiding this comment.
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.
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.
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.
failover2 connect() is called, then getVerifiedConnection() is called.
After that, if connection isn't established, a failover starts.
And eventually it calls pluginService.connect()
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.
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
sergiyvamz
force-pushed
the
failover2-on-connect
branch
from
b84d66c to
df3d833
Compare
3 participants
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.