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

build: add feature flag private-link #15368

Closed
wants to merge 2 commits into from
Closed

build: add feature flag private-link #15368

wants to merge 2 commits into from

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Feb 29, 2024

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 to cargo clean to clean disk space...

Alternative solution is to replace aws-sdk-ec2 with manual HTTP requests (like opendal). 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

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

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.

@xxchan xxchan requested a review from a team as a code owner February 29, 2024 08:09
Copy link
Member Author

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

@xxchan xxchan changed the title refactor: add feature flag private-link build: add feature flag private-link Feb 29, 2024
@@ -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
Copy link
Contributor

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:

  1. ./risedev d
  2. docker image
  3. 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.

Copy link
Member Author

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..

@xxchan xxchan closed this Mar 6, 2024
@xxchan xxchan deleted the xxchan/private-link branch April 18, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants