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

add virtual targets read/write #4397

Closed
wants to merge 3 commits into from
Closed

Conversation

replay
Copy link
Contributor

@replay replay commented Sep 30, 2021

Add virtual targets named read/write/async. Each of them represents one component in a typical 3 binary deployment, where read contains all services that are part of the read path, write contains all services that are part of the write path, and async contains all services that act as asynchronous background jobs.

These are aliases to improve the ease of use when a user wants to create a 3 component deployment.

Edit 2021-10-07 15:59: the target async has been removed as of #4397 (comment)

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2021

CLA assistant check
All committers have signed the CLA.

@replay replay marked this pull request as ready for review September 30, 2021 18:29
@replay replay requested a review from a team as a code owner September 30, 2021 18:29
@09jvilla
Copy link
Contributor

09jvilla commented Oct 1, 2021

@slim-bean should "TableManager" be included in "async"? I thought that with the newest versions of Loki, TableManager was no longer needed since its functionality was handled by the compactor.

Just want to make sure we're pushing/defaulting people to the deployment we recommend (so not sure if that does or doesn't include that component). (I guess you could alternatively include it in the target and default it to not doing anything / being off).

cc @replay -- copying this question over from our slack thread

@replay
Copy link
Contributor Author

replay commented Oct 1, 2021

I removed the Table Manager, thx for pointing that out

@trevorwhitney
Copy link
Collaborator

My understanding is that the table manager is necessary for a "chunks" back-end like cassandra or big table, but is not necessary if using object storage as the back-end. If that's true, I don't think it makes sense to add a table manager to these new targets.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

LGTM, but do we have a solution for registering the query schedulers with the frontends and the queriers with the schedulers? If not we should make sure we capture that in a future story (also looks like it's on the agenda for Monday's meeting).

@replay
Copy link
Contributor Author

replay commented Oct 1, 2021

do we have a solution for registering the query schedulers with the frontends and the queriers with the schedulers

As far as I remember, the last time we discussed this we said that if each of them just connect to localhost then this should be "good enough" for a basic setup.

@trevorwhitney
Copy link
Collaborator

TBH I'm confused on what state the cluster was in when we were testing. IIRC, we had a scheduler in place when @replay was running his tests, and that without a scheduler we'd only be able to achieve 1GB/s query performance max. Also, if the schedulers are not communicating with each other, what's the point in having them as the frontend can do scheduling for a co-located querier? I think if we're planning to add the schduler, we need a solution to cluster them, but please correct me if I'm misunderstanding something.

@replay
Copy link
Contributor Author

replay commented Oct 1, 2021

IIRC, we had a scheduler in place when @replay was running his tests

At the time I removed the singleton pod and I made sure that the read pods had the targets query-frontend,query-scheduler,querier. So i think this means that each had a local query scheduler. But to be sure i guess we'll have to check with ed

@replay
Copy link
Contributor Author

replay commented Oct 6, 2021

I updated this based on our latest discussion in https://docs.google.com/document/d/1egEK3w0K6RMGP4fPHV28G5ocxrJp4f2o1f7MxyqRkFc/edit#

image

@replay
Copy link
Contributor Author

replay commented Oct 6, 2021

@trevorwhitney I know you already approved, but I made changes in the meantime since then so I wanted to give you another chance to take a look before merging

@slim-bean
Copy link
Collaborator

I'd like to hold off merging this until #4424 is merged to make the rebase a little easier.

@replay replay changed the title add virtual targets read/write/async add virtual targets read/write Oct 7, 2021
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

Looks great, agreed on holding off until the query-scheduler ring work is done. Thanks!

@replay
Copy link
Contributor Author

replay commented Oct 12, 2021

I'd like to hold off merging this until #4424 is merged to make the rebase a little easier.

I'm not sure it really makes a difference for the rebase whether we merge this first or later. This PR really just adds a few lines and it should be easy to rebase on top of it, I'm happy to help if the rebase turns out to be difficult.
Consider that this has now been blocked for 5 days already, can we reevaluate whether it should remain blocked?

@trevorwhitney
Copy link
Collaborator

closing in favor of #4498, which builds off this work

@DylanGuedes DylanGuedes deleted the 2007_create_virtual_targets branch February 24, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants