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

fix: remove retries for instance-lookup #802

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Suraiya-Hameed
Copy link
Member

Removed option.retries from instance-lookup.js, this option does not exist and it is never passed from connection.js.
Also, fixes #672

removed option.retrries from instance-lookup.js, this option does not exist and it is never passed from connection.js.
@arthurschreiber
Copy link
Collaborator

@Suraiya-Hameed I'm wondering why we even had this option in the first place. 🤔

@Suraiya-Hameed
Copy link
Member Author

Same here. From issue #195, probably the were intending to remove retries but missed it!

@IanChokS IanChokS added the Action Required An action is needed by Tedious member label Dec 16, 2019
@IanChokS IanChokS requested a review from arthurschreiber March 4, 2020 23:57
Copy link
Member

@IanChokS IanChokS left a comment

Choose a reason for hiding this comment

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

This is an old PR but I believe the fix is still valid.

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #802 into master will decrease coverage by 0.92%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
- Coverage   80.77%   79.84%   -0.93%     
==========================================
  Files          86       86              
  Lines        4384     4376       -8     
  Branches      793      791       -2     
==========================================
- Hits         3541     3494      -47     
- Misses        584      626      +42     
+ Partials      259      256       -3     
Impacted Files Coverage Δ
src/connection.ts 60.59% <0.00%> (-3.73%) ⬇️
src/instance-lookup.ts 100.00% <100.00%> (+3.70%) ⬆️
src/incoming-message-stream.ts 92.30% <0.00%> (-5.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fb901a...35ff939. Read the comment docs.

@IanChokS
Copy link
Member

IanChokS commented Mar 10, 2020

@arthurschreiber eslint test is failing because of previous commit messages not following commitlint convention. Not sure how to fix. But overall this PR has been updated 😄

@arthurschreiber
Copy link
Collaborator

@IanChokS I double checked this. This removes the retries option, and also completely removes the retries. This might be problematic, as the server instance lookup is done via UDP, which is not reliable and can have packets silently dropped.

I think maybe we should remove the retries option, but also exposing a way to abort the lookup? Then we could just keep on retrying until we hit the connection timeout. @IanChokS @MichaelSun90 what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action Required An action is needed by Tedious member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unavailable SQL_SERVER_BROWSER_PORT gets wrong error message
4 participants