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

Zone aware ingesters #1936

Merged
merged 5 commits into from Dec 15, 2022
Merged

Conversation

manohar-koukuntla
Copy link
Contributor

@manohar-koukuntla manohar-koukuntla commented Dec 5, 2022

What this PR does:
Add zone aware ingesters.

Which issue(s) this PR fixes:
Fixes #1578

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Dec 5, 2022

CLA assistant check
All committers have signed the CLA.

@manohar-koukuntla manohar-koukuntla force-pushed the zone_aware_ingesters branch 2 times, most recently from 0ea1509 to 7687060 Compare December 5, 2022 10:20
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Thank you for this addition the jsonnet as well as a bit of cleanup. I had one small comment so far.

This is an impressive feature addition that uses features of the ring I have not had a chance to dig into myself. I have never run zone aware ingesters in Tempo so it's going to take me a bit longer to review this PR. Likely I will need to just run this locally and confirm everything is working as expected. Primarily commenting to let you know this is on my list and will get a proper review shortly.

@manohar-koukuntla
Copy link
Contributor Author

manohar-koukuntla commented Dec 7, 2022

Thank you for this addition the jsonnet as well as a bit of cleanup. I had one small comment so far.

This is an impressive feature addition that uses features of the ring I have not had a chance to dig into myself. I have never run zone aware ingesters in Tempo so it's going to take me a bit longer to review this PR. Likely I will need to just run this locally and confirm everything is working as expected. Primarily commenting to let you know this is on my list and will get a proper review shortly.

Thank you for letting me know.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Did some basic testing on this. The basic jsonnet started up fine and everything was working.

However, when I added _config.multi_zone_ingster_enabled: true I received this error:

Error: evaluating jsonnet: RUNTIME ERROR: Unexpected type null
	During manifestation	

By setting both multi_zone_ingester_enabled and multi_zone_ingester_migration_enabled to true I was able to get it to render. Before I go further can we get a readme in ./operations/jsonnet/microservices that describes how to use these fields correctly?

Honestly we should already have one to help people make sense of the jsonnet. This will be a good start.

@manohar-koukuntla
Copy link
Contributor Author

Did some basic testing on this. The basic jsonnet started up fine and everything was working.

However, when I added _config.multi_zone_ingster_enabled: true I received this error:

Error: evaluating jsonnet: RUNTIME ERROR: Unexpected type null
	During manifestation	

By setting both multi_zone_ingester_enabled and multi_zone_ingester_migration_enabled to true I was able to get it to render. Before I go further can we get a readme in ./operations/jsonnet/microservices that describes how to use these fields correctly?

Honestly we should already have one to help people make sense of the jsonnet. This will be a good start.

This is because here it is using tempo_ingester_statefulset is null when only multi_zone_ingester_enabled is enabled. I ll just a another if condition in the example but Ideally tempo_ingester_statefulset should not be used when only multi_zone_ingester_enabled is enabled.

@joe-elliott
Copy link
Member

Thanks for the fix, but I would still like a description of how to use these config values in a readme. Can you create ./operations/jsonnet/microservices/readme.md. With content like:

# Tempo Microservices Jsonnet

## Multizone
< how to use multizone configuration here >

This will help me and others understand how to use the 4 added config values if they want to use multizone.

@manohar-koukuntla
Copy link
Contributor Author

Thanks for the fix, but I would still like a description of how to use these config values in a readme. Can you create ./operations/jsonnet/microservices/readme.md. With content like:

# Tempo Microservices Jsonnet

## Multizone
< how to use multizone configuration here >

This will help me and others understand how to use the 4 added config values if they want to use multizone.

@joe-elliott added new readme file.

@manohar-koukuntla
Copy link
Contributor Author

Any update on this?

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Wow, I didn't mean to document using the jsonnet itself. I was just hoping for a few lines on the various multizone configuration options. I've run this locally and it LGTM.

Thanks!

@joe-elliott joe-elliott merged commit e96d482 into grafana:main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Zone aware replication
3 participants