columnar: fix CD compile error#10887
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
@JaySon-Huang 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/L
|
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:
📝 WalkthroughWalkthroughConvert kvproto and tipb git sources from SSH to HTTPS and centralize their crate patches in the root ChangesDependency & Build Script Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
|
/test ? |
|
@JaySon-Huang: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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. |
|
/test pull-integration-next-gen-columnar |
![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
🤖 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/Cargo.toml`:
- Line 32: The git dependencies for kvproto and tipb are currently floating;
update the Cargo.toml entries that declare kvproto = { git =
"https://github.com/pingcap/kvproto.git" } and tipb = { git =
"https://github.com/pingcap/tipb.git" } to include pinned rev fields from the
Cargo.lock: set kvproto rev = "1e6ff1c80df24a9ead2b4408133af23c09273718" and
tipb rev = "a4d204a193b4f9aa776343ac75281cf8442343ba" so the protobuf-generated
APIs used by run.rs and columnar_impls.rs remain stable; update each Cargo.toml
where those git deps are declared to include the corresponding rev string.
🪄 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: d51e9a5e-d92d-4317-a91c-fbcff7e743d5
⛔ Files ignored due to path filters (1)
contrib/tiflash-columnar-hub/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
contrib/tiflash-columnar-hub/Cargo.tomlcontrib/tiflash-columnar-hub/hub-runtime/Cargo.tomlcontrib/tiflash-columnar-hub/hub-runtime/src/profile.rs
✅ Files skipped from review due to trivial changes (1)
- contrib/tiflash-columnar-hub/hub-runtime/src/profile.rs
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
release-linux-llvm/scripts/build-tiflash-release.sh (1)
113-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not unset
LD_LIBRARY_PATHfor the remainder of the script.Line 114 now runs before the non-columnar configure/build block, so the relocated flow strips
LD_LIBRARY_PATHfrom the latercmakeandcmake --buildsteps as well. That changes the build environment, not just the post-install verification.Suggested fix
- unset LD_LIBRARY_PATH - readelf -d "${INSTALL_DIR}/tiflash" - ldd "${INSTALL_DIR}/tiflash" + env -u LD_LIBRARY_PATH readelf -d "${INSTALL_DIR}/tiflash" + env -u LD_LIBRARY_PATH ldd "${INSTALL_DIR}/tiflash" # show version - ${INSTALL_DIR}/tiflash version + env -u LD_LIBRARY_PATH "${INSTALL_DIR}/tiflash" version🤖 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 `@release-linux-llvm/scripts/build-tiflash-release.sh` around lines 113 - 120, The script currently unsets LD_LIBRARY_PATH globally (unset LD_LIBRARY_PATH) before running readelf/ldd/${INSTALL_DIR}/tiflash version which removes needed library paths for subsequent cmake and build steps; instead limit this change to the verification step by either (a) running readelf/ldd and ${INSTALL_DIR}/tiflash version inside a subshell where LD_LIBRARY_PATH is unset, or (b) save the original LD_LIBRARY_PATH into a temp var and restore it immediately after the readelf/ldd/version checks so that later cmake and cmake --build steps see the original LD_LIBRARY_PATH; locate the unset invocation and the surrounding verification commands (readelf, ldd, ${INSTALL_DIR}/tiflash version) and implement one of these scoped approaches to avoid wiping LD_LIBRARY_PATH for the remainder of the script.
🤖 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 `@release-linux-llvm/scripts/build-tiflash-release.sh`:
- Around line 63-69: Change the unconditional else branch to apply the
failpoint/jemalloc overrides only for sanitizer builds: keep the RELWITHDEBINFO
branch setting ENABLE_FAILPOINTS="OFF" and JEMALLOC_NARENAS="-1", add an elif
that checks if CMAKE_BUILD_TYPE equals "ASAN" or "TSAN" and only in that case
set ENABLE_FAILPOINTS="ON" and JEMALLOC_NARENAS="40"; for other build types
leave ENABLE_FAILPOINTS and JEMALLOC_NARENAS unset so CMake defaults
(ENABLE_TESTS/jemalloc defaults) remain in effect.
---
Outside diff comments:
In `@release-linux-llvm/scripts/build-tiflash-release.sh`:
- Around line 113-120: The script currently unsets LD_LIBRARY_PATH globally
(unset LD_LIBRARY_PATH) before running readelf/ldd/${INSTALL_DIR}/tiflash
version which removes needed library paths for subsequent cmake and build steps;
instead limit this change to the verification step by either (a) running
readelf/ldd and ${INSTALL_DIR}/tiflash version inside a subshell where
LD_LIBRARY_PATH is unset, or (b) save the original LD_LIBRARY_PATH into a temp
var and restore it immediately after the readelf/ldd/version checks so that
later cmake and cmake --build steps see the original LD_LIBRARY_PATH; locate the
unset invocation and the surrounding verification commands (readelf, ldd,
${INSTALL_DIR}/tiflash version) and implement one of these scoped approaches to
avoid wiping LD_LIBRARY_PATH for the remainder of the script.
🪄 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: 7911bb3e-1cba-4480-bd0a-8a0debf3bc57
⛔ Files ignored due to path filters (1)
contrib/tiflash-columnar-hub/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
contrib/tiflash-columnar-hub/Cargo.tomlcontrib/tiflash-columnar-hub/hub-runtime/Cargo.tomlrelease-linux-llvm/scripts/build-tiflash-release.sh
Signed-off-by: JaySon-Huang <tshent@qq.com>
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contrib/tiflash-columnar-hub/Cargo.toml (1)
27-28: ⚡ Quick winReproducibility for kvproto/tipb git patches (Cargo.toml):
contrib/tiflash-columnar-hub/Cargo.tomlpointskvproto/tipbat git withoutrev/tag, butcontrib/tiflash-columnar-hub/Cargo.lockalready pins them (kvproto→1e6ff1c80df24a9ead2b4408133af23c09273718,tipb→a4d204a193b4f9aa776343ac75281cf8442343ba), and the Makefile builds withcargo build --locked, so builds stay reproducible as long asCargo.lockis kept in sync when changing these deps. Optional: consider addingrevinCargo.tomlfor clarity.🤖 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/Cargo.toml` around lines 27 - 28, The Cargo.toml currently specifies kvproto and tipb as git dependencies without pinned revs which can be unclear; ensure reproducible builds by either (A) keeping Cargo.lock updated whenever those git dependencies change (verify contrib/tiflash-columnar-hub/Cargo.lock contains the commits kvproto=1e6ff1c80df24a9ead2b4408133af23c09273718 and tipb=a4d204a193b4f9aa776343ac75281cf8442343ba and maintain that lock in the repo because the Makefile invokes cargo build --locked), or (B) explicitly add rev entries to the kvproto and tipb git specifications in contrib/tiflash-columnar-hub/Cargo.toml to pin those commits for clarity and reproducibility; pick one approach and update either Cargo.lock or Cargo.toml accordingly.
🤖 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 `@contrib/tiflash-columnar-hub/Cargo.toml`:
- Around line 27-28: The Cargo.toml currently specifies kvproto and tipb as git
dependencies without pinned revs which can be unclear; ensure reproducible
builds by either (A) keeping Cargo.lock updated whenever those git dependencies
change (verify contrib/tiflash-columnar-hub/Cargo.lock contains the commits
kvproto=1e6ff1c80df24a9ead2b4408133af23c09273718 and
tipb=a4d204a193b4f9aa776343ac75281cf8442343ba and maintain that lock in the repo
because the Makefile invokes cargo build --locked), or (B) explicitly add rev
entries to the kvproto and tipb git specifications in
contrib/tiflash-columnar-hub/Cargo.toml to pin those commits for clarity and
reproducibility; pick one approach and update either Cargo.lock or Cargo.toml
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b9b9c5d-3346-4f9b-a754-f600b2511adc
⛔ Files ignored due to path filters (1)
contrib/tiflash-columnar-hub/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.gitmodulescontrib/cloud-storage-enginecontrib/tiflash-columnar-hub/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- .gitmodules
|
@JaySon-Huang: 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. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Lloyd-Pottiger, yongman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: ref #10844
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit