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

Default wal to enabled in jsonnet lib #4629

Merged
merged 2 commits into from
Nov 3, 2021

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Nov 3, 2021

Followup to #4624 now that the wal is an enabled configuration by default.

@owen-d owen-d requested a review from a team as a code owner November 3, 2021 12:43
Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM

query_scheduler_enabled: false,
overrides_exporter_enabled: false,

// flags for running ingesters/queriers as a statefulset instead of deployment type.
// WAL enabled configurations automatically use statefulsets.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this comment is saying?

Copy link
Member Author

Choose a reason for hiding this comment

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

The jsonnet lib tries to detect if a WAL is being used and if so, use k8s statefulsets instead of deployments and I wanted to leave a note that mentioned this explicitly. The relevant code I'm referring to is here: https://github.com/grafana/loki/blob/main/production/ksonnet/loki/wal.libsonnet#L7

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. with the line above it's ambiguous if its something that requires stateful queriers (vs just ingesters).

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment precedes the stateful_ingesters line, which is what it's actually commenting (I also find the github display for this a bit weird):
image

@owen-d owen-d merged commit e8bd3cb into grafana:main Nov 3, 2021
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.

None yet

3 participants