-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Expose Endpoint Info timeout parameter #5480
Conversation
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.
It makes sense to introduce this parameter, but I'm a bit wary of introducing new separate flags in face of work being done in #5340. I assume one of the results will be that we will have a comprehensive endpoint configuration, instead of having individual flags. I'm just wondering if this would not mean we introduce a new flag only to deprecate it couple of releases later, when the endpoint.config
will potentially supersede it. Maybe having this as a hidden flag first would make sense and then move it to the endpoint config. But maybe @bwplotka, @saswatamcode or @SrushtiSapkale can give us more qualified answer!
Good to know we have WIP work on that. If that's the case yeah definitely better than adding this new flag separately. |
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.
Yeah... maybe let's make it hidden for now? 🤔
a9ceb15
to
9bc5d41
Compare
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
9bc5d41
to
f897724
Compare
Signed-off-by: Ben Ye <[email protected]>
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.
Looks like one more conflict, should be good to go afterwards!
* Expose Endpoint Info timeout parameter Signed-off-by: Ben Ye <[email protected]> * fix test timeout Signed-off-by: Ben Ye <[email protected]> * update doc and changelog Signed-off-by: Ben Ye <[email protected]> * make it a hidden flag Signed-off-by: Ben Ye <[email protected]> * rebase main Signed-off-by: Ben Ye <[email protected]> * fix tests Signed-off-by: Ben Ye <[email protected]> Signed-off-by: Ben Ye <[email protected]> Co-authored-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Ben Ye [email protected]
Changes
Fixes #5450
Expose info timeout param so that users can configure it based on their use case use.
For our use case, we have Thanos Query at cloud control plane and Thanos Sidecars at edge across the world (like Asia, Africa, etc). Due to the physical location distance and network issues, 5 seconds timeout is not sufficient for all cases.
Verification