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

feat(BA-24): Split redis config for each connection pool #3725

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

seedspirit
Copy link
Contributor

resolves #3195 (BA-24)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

@github-actions github-actions bot added comp:common Related to Common component comp:installer Related to Installer size:S 10~30 LoC labels Feb 17, 2025
@seedspirit seedspirit changed the title refactor(BA-24): Split redis instance refactor(BA-24): Split redis config for each connection pool Feb 17, 2025
@github-actions github-actions bot added size:L 100~500 LoC and removed size:S 10~30 LoC labels Feb 18, 2025
@github-actions github-actions bot added comp:manager Related to Manager component comp:agent Related to Agent component comp:cli Related to CLI component comp:webserver Related to Web Server component labels Feb 19, 2025
@seedspirit seedspirit marked this pull request as ready for review February 20, 2025 05:07
Comment on lines +1299 to +1305
STANDARD_FIELDS = frozenset({
"addr",
"sentinel",
"service_name",
"password",
"redis_helper_config",
})
Copy link
Collaborator

@HyeockJinKim HyeockJinKim Feb 21, 2025

Choose a reason for hiding this comment

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

Why is this setting necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As redis config allows extra fields, we need to add what is standard and what is not standard fields.
The only way I know of to add a field that is not predefined in the dataclass is to have it as a set or dict, as it is currently. Is there any other way?

redis_config_iv = t.Dict({
    t.Key("addr", default=redis_default_config["addr"]): t.Null | tx.HostPortPair,
    ...
    t.Key("override_configs", default=None): t.Null
    | t.Mapping(
        t.String,
        t.Dict({
            t.Key("addr", default=redis_default_config["addr"]): t.Null | tx.HostPortPair,
            ...
        }).allow_extra("*"),
    ),
}).allow_extra("*")

@seedspirit seedspirit changed the title refactor(BA-24): Split redis config for each connection pool feat(BA-24): Split redis config for each connection pool Feb 21, 2025
Comment on lines +15 to +20
STAT = "stat"
RLIM = "rlim"
LIVE = "live"
IMAGE = "image"
STREAM = "stream"
STREAM_LOCK = "stream_lock"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid using abbreviations like RLIM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:agent Related to Agent component comp:cli Related to CLI component comp:common Related to Common component comp:installer Related to Installer comp:manager Related to Manager component comp:webserver Related to Web Server component size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split Redis configuration for each connection pool by database
2 participants