Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
df3d833
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?
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, thengetVerifiedConnection()
is called.aws-advanced-jdbc-wrapper/wrapper/src/main/java/software/amazon/jdbc/plugin/failover2/FailoverConnectionPlugin.java
Line 654 in b84d66c
After that, if connection isn't established, a failover starts.
aws-advanced-jdbc-wrapper/wrapper/src/main/java/software/amazon/jdbc/plugin/failover2/FailoverConnectionPlugin.java
Line 664 in b84d66c
And eventually it calls
pluginService.connect()
aws-advanced-jdbc-wrapper/wrapper/src/main/java/software/amazon/jdbc/plugin/failover2/FailoverConnectionPlugin.java
Line 507 in b84d66c
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
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.
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 infailover2
. SoforceConnect()
makes not much sense to this case. Also,failover2
uses regularconnect()
becauseMonitoringHostListProvider
already verified topology so no need to force direct connect. And with a regularconnect()
, the call comes tofailover2
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.