Skip to content

Conversation

@KMiller-Grafana
Copy link
Contributor

This new reference info on consistent hash rings is incomplete. Please use comments to suggest the missing pieces and identify incorrect information.

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! I added some suggestions for what the query-scheduler and compactor rings do, though might want @slim-bean to confirm what I said about the query-scheduler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The query schedulers use the information in their key-value store to allow enable "service discovery" of the schedulers. This allows query workers to connect to all available schedulers, and scheduler to connect to all available query frontends, effectively creating a single queue that query load can be sharded across.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Compactors use the information in the key-value store to identify a single instance that will be responsible for compaction, and the compactor is only enabled on that instance, despite the target being enabled on multiple instances.

@KMiller-Grafana KMiller-Grafana marked this pull request as ready for review December 9, 2021 19:55
@KMiller-Grafana KMiller-Grafana requested a review from a team December 9, 2021 19:55
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Added some minor suggestions but other than that it looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

The png is not referenced correctly

Suggested change
![distributor and ingester rings](../ring-overview.png)
![distributor and ingester rings](./ring-overview.png)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandeepsukhani : this image callout is correct as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each node has a key-value store that holds communication information
Each node has a key-value pair in the store that holds communication information

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'm leaving this as is. My prose works top down, and it states that each node in the ring has a store. It is also true that each node is represented as a key-value pair within the store, but that isn't what I wished to convey.

Comment on lines 39 to 40
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks out of the context because gossip(memberlist) is one of the supported stores for the ring. You can maybe say each node periodically keeps updating its info in the store at the configured heartbeat interval.

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 don't understand your comment. To me, there are 2 different issues. Issue 1: which store is used. Issue 2: the gossip protocol is used to propagate info.

gossip(memberlist) is a particular implementation detail and not about how the rings work (in general).

Are you telling me that a different algorithm is used to propagate info if a store type other than memberlist is used? If yes, then I will change the prose.

My prose is there to explain to readers the basics of how the ring works. I'm trying to do this independent of which configuration/implementation they choose for their key-value store.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think not all available stores use the gossip protocol (ex: etcd and inmemory).

@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 22, 2021
@KMiller-Grafana KMiller-Grafana merged commit bd1fef0 into grafana:main Dec 22, 2021
@KMiller-Grafana KMiller-Grafana deleted the docs/document-rings branch December 22, 2021 22:22
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