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

Implement Granular Endpoint Configuration #5585

Closed
wants to merge 3 commits into from

Conversation

SrushtiSapkale
Copy link
Contributor

Signed-off-by: Srushti Sapkale srushtiisapkale@gmail.com

This PR is a part of implementing granular endpoint configuration
Proposal WIP #5505

  • Added --endpoint.config flag

pkg/extgrpc/config.go Outdated Show resolved Hide resolved
@SrushtiSapkale SrushtiSapkale force-pushed the endpoints branch 4 times, most recently from 5f341e7 to 6c6e23e Compare August 27, 2022 19:17
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome work @SrushtiSapkale! 🌟

Added some comments! 🙂

cmd/thanos/query.go Outdated Show resolved Hide resolved
cmd/thanos/query.go Outdated Show resolved Hide resolved
pkg/extgrpc/grpc.go Outdated Show resolved Hide resolved
pkg/query/config.go Outdated Show resolved Hide resolved
cfg := EndpointConfig{}
cfg.Endpoints = endpointAddrs
if globalFileSDConfig.Files != nil {
cfg.EndpointsSD = []httpconfig.FileSDConfig{
Copy link
Member

Choose a reason for hiding this comment

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

So we can maybe add a new field to the EndpointConfig struct, something like EndpointSDDiscoverer and in LoadConfig we can use file.NewDiscovery to process all the file SD configs and populate the new field. We would then be able to use the discoverers directly in query.go with fileSDCache. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding endpointSDDiscoverer in query.go and the test is passing, wdyt is more preferable? 😅

test/e2e/query_test.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@SrushtiSapkale SrushtiSapkale left a comment

Choose a reason for hiding this comment

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

Hello @saswatamcode thanks a lot for the feedback, I also tried making some changes. Can u please review if they are helpful or we should keep it the way you suggested 😄

Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
Signed-off-by: Srushti Sapkale <srushtiisapkale@gmail.com>
@stale
Copy link

stale bot commented Oct 30, 2022

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Oct 30, 2022
@stale
Copy link

stale bot commented Nov 12, 2022

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants