Add standalone compactors flag and service charts#171
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e3a777d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
shuheiktgw
left a comment
shuheiktgw
left a comment
There was a problem hiding this comment.
I left one comment but basically LGTM!
| # happen on the indexers exactly as before. Because the same value drives both | ||
| # the env var and whether the workload is rendered, the compactor pods exist if | ||
| # and only if this flag is true. | ||
| enableStandaloneCompactors: false |
There was a problem hiding this comment.
I believe this should be compactor.enabled, to follow the convention used by the other services.
There was a problem hiding this comment.
The reason for this naming is that it doesn't gate the service - it gates an entire set of functionality, that changes how indexers, the janitor, and the new compactor service work. It gets translated into an env var. From there, we have the actual compactor service itself, which should follow the convention you mentioned.
There was a problem hiding this comment.
I see the reasoning. I personally still think compactor.enabled is the better public API because it is more consistent with the rest of the chart. IIUC, since enabling the compactor necessarily enables the standalone compactor, having compactor.enabled also inject QW_ENABLE_STANDALONE_COMPACTORS into the other components feels reasonable to me, as long as we document that clearly in values.yaml 🙂
nadav-govari
force-pushed
the
nadav/compactors
branch
from
93e3a77 to
a55923a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a55923a08c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
This change is alongside quickwit-oss/quickwit#6461. It adds the enable_standalone_compactor flag. When enabled, it affects three Quickwit services:
As far as the helm chart goes, when false this change is a no-op. When true, it adds the
compactorservice and defines it similarly to other deployments in this chart. It also injects the QW_ENABLE_STANDALONE_COMPACTORS env var to all services as true.