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

Refactor Idle Connection Resiliency timeout to use existing SharedTimer #1794

Merged
merged 6 commits into from
Apr 14, 2022

Conversation

David-Engel
Copy link
Contributor

@David-Engel David-Engel commented Apr 7, 2022

The ICR feature brought in a previous timeout implementation that had drawbacks. This changes ICR to use the current SharedTimer, which is much more efficient.

This PR obsoletes #1782 and #1789 since it completely removes the class being updated in those two PRs.

EDIT: While testing, discovered that ICR also fails to reconnect a connection that has already been recovered once by ICR. This is also fixed in this PR.

@David-Engel David-Engel added this to the 11.1.1 milestone Apr 7, 2022
@lilgreenbird lilgreenbird changed the title Refactor ICR timeout to use existing SharedTimer Refactor Idle Connection Resiliency timeout to use existing SharedTimer Apr 8, 2022
@lilgreenbird lilgreenbird added this to Under Investigation in MSSQL JDBC via automation Apr 8, 2022
this.command = cmd;
connectRetryCount = con.getRetryCount();
eReceived = null;
stopRequested = false;
if (loggerExternal.isLoggable(Level.FINER)) {
loggerExternal.finer("ICR reconnect thread initialized. Connection retry count = " + connectRetryCount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use ICR acronym? (multiple occurences)

while ((connectRetryCount != 0) && (!stopRequested) && keepRetrying) {
while ((connectRetryCount > 0) && (!stopRequested) && keepRetrying) {
if (loggerExternal.isLoggable(Level.FINER)) {
loggerExternal.finer("Running ICR reconnect for command: " + command.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] combine both to just 1 call?

Jeffery-Wasty
Jeffery-Wasty previously approved these changes Apr 12, 2022
VeryVerySpicy
VeryVerySpicy previously approved these changes Apr 12, 2022
tkyc
tkyc previously approved these changes Apr 12, 2022
@lilgreenbird lilgreenbird moved this from Under Investigation to Under Peer Review in MSSQL JDBC Apr 14, 2022
@lilgreenbird lilgreenbird merged commit 487c355 into microsoft:main Apr 14, 2022
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

5 participants