-
Notifications
You must be signed in to change notification settings - Fork 590
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
build: add feature flag private-link #15368
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: can hide whitespace changes to review
@@ -63,8 +63,8 @@ if [[ -n "${BUILDKITE_TAG}" ]]; then | |||
fi | |||
|
|||
echo "--- Build risingwave release binary" | |||
cargo build -p risingwave_cmd_all --features "rw-static-link" --profile release | |||
cargo build --bin risectl --features "rw-static-link" --profile release | |||
cargo build -p risingwave_cmd_all --features rw-static-link --features private-link --profile release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clarify how we handle the private-link
flag in different environment/setups, such as:
- ./risedev d
- docker image
- single-node mode, etc.
What I concern is that this flag may introduce inconsistent between different setups and we don't have CI to cover the functionality test yet. And we should not trade functionality for faster compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#15379 I have another solution which doesn't introduce feature flag.
Some thoughts about feature flag:
I admit that feature flag is not easy to use, but since @wangrunji0408 already introduced a feature flag in #15328, adding more features don't make the situation worse. IMO 1 feature is no much difference from N features.. (This is also why I raised this PR at this time)
It's not that hard to get it correct for production (just release.sh
and Dockerfile
), and we just need to get it correct once. So I would say we are not "trading functionality" at all. The benefit of faster compile time is too HUGE to ignore..
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
The main motivation is to make the dependency
aws-sdk-ec2
optional.It is very rarely used in development, but contributes to more than half of the compile time.
Although in theory, we only need to compile it once during development, but in practice,
workspace-hack
doesn't work well and it can be recompiled due to many reasons. Besides, we often need tocargo clean
to clean disk space...Alternative solution is to replace
aws-sdk-ec2
with manual HTTP requests (likeopendal
). But I think feature flag is easier and safer to do, especially since I'm not very familiar with private link and there are no tests.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.