contrib: fix columnar hub IaConfig default value#10864
Conversation
Signed-off-by: yongman <yming0221@gmail.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[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 |
|
@yongman I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/M
📝 WalkthroughWalkthrough
ChangesConfigFile Default Implementation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contrib/tiflash-columnar-hub/hub-runtime/src/run.rs (1)
1428-1447: 💤 Low valueConsider adding test coverage for non-Linux/macOS platforms.
The test only validates Linux and macOS defaults. If you add a fallback for other operating systems (per the critical issue above), consider adding a test branch for those platforms as well.
📝 Optional test enhancement
#[test] fn test_config_file_default_uses_proxy_compatible_ia_defaults() { let config = ConfigFile::default(); #[cfg(target_os = "linux")] { assert_eq!(config.ia.mem_cap, AbsoluteOrPercentSize::Percent(20.0)); assert_eq!(config.ia.disk_cap, AbsoluteOrPercentSize::Percent(70.0)); } #[cfg(target_os = "macos")] { assert_eq!( config.ia.mem_cap, AbsoluteOrPercentSize::Abs(ReadableSize::mb(100)) ); assert_eq!( config.ia.disk_cap, AbsoluteOrPercentSize::Abs(ReadableSize::mb(100)) ); } + #[cfg(not(any(target_os = "linux", target_os = "macos")))] + { + // Fallback platforms should use conservative absolute defaults + assert_eq!( + config.ia.mem_cap, + AbsoluteOrPercentSize::Abs(ReadableSize::mb(100)) + ); + assert_eq!( + config.ia.disk_cap, + AbsoluteOrPercentSize::Abs(ReadableSize::mb(100)) + ); + } }🤖 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 `@contrib/tiflash-columnar-hub/hub-runtime/src/run.rs` around lines 1428 - 1447, The test test_config_file_default_uses_proxy_compatible_ia_defaults only asserts Linux and macOS behavior; add a third cfg branch for non-Linux/macOS targets (e.g. #[cfg(not(any(target_os = "linux", target_os = "macos")))]) that constructs ConfigFile::default() and asserts config.ia.mem_cap and config.ia.disk_cap equal the intended fallback AbsoluteOrPercentSize values (use the same enum variants as in other branches); reference ConfigFile::default, config.ia.mem_cap, config.ia.disk_cap, AbsoluteOrPercentSize, and ReadableSize::mb when mirroring the expected values.
🤖 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.
Inline comments:
In `@contrib/tiflash-columnar-hub/hub-runtime/src/run.rs`:
- Around line 172-184: The function update_default_ia_config is only defined for
linux and macos causing build failures on other targets where it's referenced;
add a portable fallback or conditional call: either provide a non-conditional
implementation of update_default_ia_config(ia: &mut IaConfig) that sets sane
defaults for other platforms, or wrap the site that calls
update_default_ia_config in the same #[cfg(...)] checks so it's only called
where the function exists; refer to the update_default_ia_config function and
the IaConfig type to locate where to add the fallback or the cfg gating.
---
Nitpick comments:
In `@contrib/tiflash-columnar-hub/hub-runtime/src/run.rs`:
- Around line 1428-1447: The test
test_config_file_default_uses_proxy_compatible_ia_defaults only asserts Linux
and macOS behavior; add a third cfg branch for non-Linux/macOS targets (e.g.
#[cfg(not(any(target_os = "linux", target_os = "macos")))]) that constructs
ConfigFile::default() and asserts config.ia.mem_cap and config.ia.disk_cap equal
the intended fallback AbsoluteOrPercentSize values (use the same enum variants
as in other branches); reference ConfigFile::default, config.ia.mem_cap,
config.ia.disk_cap, AbsoluteOrPercentSize, and ReadableSize::mb when mirroring
the expected values.
🪄 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: dc93ab0c-30e2-40ca-808c-5bbadafc9bbf
📒 Files selected for processing (1)
contrib/tiflash-columnar-hub/hub-runtime/src/run.rs
|
@yongman: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Tests
Bug Fixes