Skip to content
This repository has been archived by the owner on Apr 2, 2024. It is now read-only.

Refactor ingestor flags to be in ingestor package #1735

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

cevian
Copy link
Contributor

@cevian cevian commented Nov 2, 2022

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:

  • CHANGELOG entry for user-facing changes
  • Updated the relevant documentation

@cevian cevian requested a review from a team as a code owner November 2, 2022 19:41
@cevian cevian force-pushed the fix_flags branch 2 times, most recently from 0794029 to d12f1ab Compare November 3, 2022 18:05
Copy link
Contributor

@niksajakovljevic niksajakovljevic left a comment

Choose a reason for hiding this comment

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

Great to separate configs so code becomes more clear. I'd only like to follow the existing convention and have one configuration struct for ingestion that is passed into ParseFlags (no need for artificial Flags struct)

type Config struct {
NumCopiers int
InvertedLabelsCacheSize uint64
Flags *Flags
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how much I like this. I'd rather keep all Ingestor configuration in one struct. Check my other comment.

"github.com/timescale/promscale/pkg/pgmodel/ingestor/trace"
)

type Flags struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really in line with existing convention. We usually define a config struct that is passed into ParseFlags to populate configurable fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants