Refactor ingestor flags to be in ingestor package #1735
Conversation
cevian
force-pushed
the
fix_flags
branch
2 times, most recently
from
0794029
to
d12f1ab
Compare
pkg/pgmodel/ingestor/ingestor.go
Outdated
| type Config struct { | ||
| NumCopiers int | ||
| InvertedLabelsCacheSize uint64 | ||
| Flags *Flags |
There was a problem hiding this comment.
Not sure how much I like this. I'd rather keep all Ingestor configuration in one struct. Check my other comment.
pkg/pgmodel/ingestor/flags.go
Outdated
| "github.com/timescale/promscale/pkg/pgmodel/ingestor/trace" | ||
| ) | ||
|
|
||
| type Flags struct { |
There was a problem hiding this comment.
This is not really in line with existing convention. We usually define a config struct that is passed into ParseFlags to populate configurable fields.
There was a problem hiding this comment.
You are right, the naming was off. I renamed this to Config and had the other struct renamed to Parameters. I do not want to combine the CLI-settable fields with the non-cli-settable fields. Thats why I keep two structs...
There was a problem hiding this comment.
I did recognize your idea but (1) It's not followed through the rest of the codebase (so this will be a snowflake) (2) The cut is not really clean because NumCopiers is configurable but it's not part of the Config struct (3) From the code perspective I am not really sure if it matters where the value is coming from (default or user) . Anyhow I'll approve the PR so you are not blocked on me.
| "github.com/timescale/promscale/pkg/version" | ||
| ) | ||
|
|
||
| // Config for the database. | ||
| type Config struct { | ||
| CacheConfig cache.Config | ||
| IngestorFlags ingestor.Config |
There was a problem hiding this comment.
nit:
| IngestorFlags ingestor.Config | |
| IngestorConfig ingestor.Config |
For consistency Config is used to refer to almost every other configuration grouping, and we already have the CacheConfig above.
Description
Cleans up the code a bit
Merge requirements
Please take into account the following non-code changes that you may need to make with your PR: