Register block-builder target in mimir #9315
Conversation
narqo
force-pushed
the
vldmr/bb-upstream-enable-target
branch
5 times, most recently
from
89a7ab2
to
6e009ae
Compare
Signed-off-by: Vladimir Varankin <[email protected]>
narqo
force-pushed
the
vldmr/bb-upstream-enable-target
branch
from
6e009ae
to
49ea29f
Compare
Signed-off-by: Vladimir Varankin <[email protected]>
narqo
force-pushed
the
vldmr/bb-upstream-enable-target
branch
from
49ea29f
to
f61c7ea
Compare
There was a problem hiding this comment.
I'm very glad you got rid of remaining offsets. LGTM.
Also, the PR addresses the discussion we had with @dimitarvdimitrov in #9199 (comment) about a potential edge-case for a partition that has offset gaps.
Can we unit test it? Or is it too complicated because we can't create gaps in the mocked Kafka cluster?
Signed-off-by: Vladimir Varankin <[email protected]>
| @@ -123,6 +124,7 @@ type Config struct { | |||
| Worker querier_worker.Config `yaml:"frontend_worker"` | |||
| Frontend frontend.CombinedFrontendConfig `yaml:"frontend"` | |||
| IngestStorage ingest.Config `yaml:"ingest_storage"` | |||
| BlockBuilder blockbuilder.Config `yaml:"block_builder" doc:"hidden"` | |||
There was a problem hiding this comment.
isn't this part of ingest storage? i mean we use the same kafka config and it's rightly coupled with kafka just like ingest storage
There was a problem hiding this comment.
I think it's fair having a root leve config. Ingest storage is the config shared between component. This config is block-builder specific.
There was a problem hiding this comment.
+1 to what Marco says. For me, ingest-storage is a package, that services like ingester, query-frontend, ruler, and now block-builder use. I.e. block-builder is not a feature of the ingest-storage package, it's the opposite.
I agree, though, that "block-builder, the service" is a feature of the "ingest-storage architecture" 🤷
| @@ -123,6 +124,7 @@ type Config struct { | |||
| Worker querier_worker.Config `yaml:"frontend_worker"` | |||
| Frontend frontend.CombinedFrontendConfig `yaml:"frontend"` | |||
| IngestStorage ingest.Config `yaml:"ingest_storage"` | |||
| BlockBuilder blockbuilder.Config `yaml:"block_builder" doc:"hidden"` | |||
There was a problem hiding this comment.
why hidden? Ingest storage is just experimental
There was a problem hiding this comment.
I don't feel strong about it, but I am not comfortable exposing anything about block-builder to the user-visible helps. Block-builder is even more experimental than ingest-storage.
For now, I cannot think of a way to replicate that in a controlled manner, so will need some exploration. I will add it to the rest of the "more tests" PRs later. |
What this PR does
This one seats atop #9199 for nowThis is part of #8635; refer to it for more details.
Here we register the
block-buildertarget in mimir. The block-builder is still in an earlier experiment, thus don't expose any documentation or changelog for now.Also, the PR addresses the discussion we had with @dimitarvdimitrov in #9199 (comment) about a potential edge-case for a partition that has offset gaps. Here we refactor the internals of the consumer loop replacing the use of remaining lag with the reference to the partition end, at the time of the cycle.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.