keyspace: get keyspace name from env#68103
Conversation
Cherry-pick the behavior from tidbcloud/tidb-cse#1909 and adapt it to the current TiDB tree. Also cover the env fallback in pkg/keyspace tests.
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
ystaticy
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @ystaticy. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGetKeyspaceNameBySettings returns the global keyspace name when set; otherwise, if in starter mode it reads ChangesKeyspace name resolution
sequenceDiagram
participant Caller
participant GetKeyspaceNameBySettings
participant OS as os.Getenv
participant Config as config.UpdateGlobal
Caller->>GetKeyspaceNameBySettings: request keyspace name
GetKeyspaceNameBySettings-->>Config: config.GetGlobalKeyspaceName() (check)
alt global empty
GetKeyspaceNameBySettings->>OS: os.Getenv(config.EnvVarKeyspaceName)
OS-->>GetKeyspaceNameBySettings: env value
GetKeyspaceNameBySettings->>Config: config.UpdateGlobal(conf.KeyspaceName = env value)
GetKeyspaceNameBySettings-->>Caller: return env value
else global set
GetKeyspaceNameBySettings-->>Caller: return global value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/keyspace/keyspace_test.go`:
- Around line 55-99: Before mutating global state with config.UpdateGlobal in
the subtests, capture a snapshot copy of the current global config (e.g., copy
the config.Config struct) and register a t.Cleanup that restores it by calling
config.UpdateGlobal(func(conf *config.Config){ *conf = savedCopy }) and also
invokes resetKeyspaceNameCache to clear the cached bytes; apply this to both
subtests that call config.UpdateGlobal so GetKeyspaceNameBySettings,
GetKeyspaceNameBytesBySettings and IsKeyspaceNameEmpty do not leave process-wide
state changed for later tests.
In `@pkg/keyspace/keyspace.go`:
- Around line 61-68: GetKeyspaceNameBytesBySettings currently reads
config.GetGlobalKeyspaceName() directly and caches that value with sync.Once,
which can permanently cache an empty string and bypass the env fallback
implemented by GetKeyspaceNameBySettings; update GetKeyspaceNameBytesBySettings
to call GetKeyspaceNameBySettings() (the string getter) and then convert that
returned string to []byte for caching, ensuring the env var fallback
(config.EnvVarKeyspaceName) is respected and avoiding a stale empty cache;
retain the sync.Once caching but base it on the string getter's result so
callers like pkg/sessionctx/stmtctx and pkg/executor get the correct value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed711854-42c3-415a-a99f-e64ff2fed247
📒 Files selected for processing (2)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68103 +/- ##
================================================
- Coverage 77.2803% 75.7524% -1.5279%
================================================
Files 2010 2008 -2
Lines 555624 565159 +9535
================================================
- Hits 429388 428122 -1266
- Misses 125316 136797 +11481
+ Partials 920 240 -680
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test check-dev2 |
|
@ystaticy: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/L
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/keyspace/keyspace.go (1)
66-73: ⚡ Quick winGate the starter/env fallback on NextGen too.
This path now consults
deploymode.IsStarter()even whenkerneltype.IsNextGen()is false, while the new tests explicitly treat deploy mode as nextgen-only. Leaving it like this makes non-nextgen behavior depend on the deploy-mode default and can still pullKEYSPACE_NAMEintoGetKeyspaceNameBySettings()unexpectedly.Suggested change
- if !deploymode.IsStarter() { + if !kerneltype.IsNextGen() || !deploymode.IsStarter() { return "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/keyspace/keyspace.go` around lines 66 - 73, The current guard only checks deploymode.IsStarter() before reading KEYSPACE_NAME and updating the global config; change the condition to require both deploymode.IsStarter() and kerneltype.IsNextGen() so the starter/env fallback is only applied for NextGen kernels. Modify the conditional around the os.Getenv(config.EnvVarKeyspaceName) and config.UpdateGlobal(...) in keyspace.go (the block that currently checks deploymode.IsStarter()) to also call kerneltype.IsNextGen(), ensuring GetKeyspaceNameBySettings() no longer picks up KEYSPACE_NAME in non-NextGen modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 66-73: The current guard only checks deploymode.IsStarter() before
reading KEYSPACE_NAME and updating the global config; change the condition to
require both deploymode.IsStarter() and kerneltype.IsNextGen() so the
starter/env fallback is only applied for NextGen kernels. Modify the conditional
around the os.Getenv(config.EnvVarKeyspaceName) and config.UpdateGlobal(...) in
keyspace.go (the block that currently checks deploymode.IsStarter()) to also
call kerneltype.IsNextGen(), ensuring GetKeyspaceNameBySettings() no longer
picks up KEYSPACE_NAME in non-NextGen modes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 021c0df6-da09-47a7-9c2c-e99984886709
📒 Files selected for processing (3)
pkg/keyspace/BUILD.bazelpkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/keyspace/BUILD.bazel
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
1 participant
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests