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
♻️ Refactors webserver's healthcheck #2910
♻️ Refactors webserver's healthcheck #2910
Changes from all commits
4420cba
605422e
0b38c4e
aaac350
4c064c3
b0549ae
a08292b
543dc64
a8aa711
1286a2a
59162ce
2a95541
a44417f
a5b7147
f205a4c
6c4d34b
bb151f1
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.
While I think it's ok to give flexibility, I don't see the benefits here.
I'd rather go for the best unit here, which looks to be milliseconds. At that point I'd rename the env var to
SC_HEALTHCHECK_TIMEOUT_MS
(pattern already used) and add a note in the description.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.
Note that heathchecks we use are of the order of seconds or tens of seconds. I do not see the need to set this in ms
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.
You are confusing me. That why even support minutes and milliseconds below since you would like to express everything in seconds?
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 agree that it would be wise to use the same timekeeing unit (MS) throughout the app. But large integer numbers also are not very clear. Difficult decision.
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.
Let's see. Here are two facts that let me take this decision:
asyncio.wait_for(coro, timeout)
where the parameter timeout is in secodsThere 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'd prefer to have especially the maximum allowed mean value not hardcoded as a magic number but settable via config. If this is the case, refer to the ENV-VAR that controls this in the comment instead of "1 sec"
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.
This was tuned after some experiments we did at a time.. I am open to change it when needed