-
Notifications
You must be signed in to change notification settings - Fork 523
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
feat(storage): support hdfs as non-s3 object store #7283
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.
Thanks for the PR!
Codecov Report
@@ Coverage Diff @@
## main #7283 +/- ##
==========================================
- Coverage 71.75% 71.73% -0.03%
==========================================
Files 1108 1109 +1
Lines 176521 176796 +275
==========================================
+ Hits 126669 126824 +155
- Misses 49852 49972 +120
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Rest LGTM! Great work!
opendal: | ||
id: opendal | ||
|
||
engine: hdfs | ||
|
||
namenode: 127.0.0.1:9000" | ||
|
||
root: risingwave |
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.
Let's add some documentations for each field like other sections.
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.
Will integrate hdfs in risedev in next PR.
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.
May remove this or at least add a TODO here.
# - use: prometheus | ||
# - use: grafana | ||
# - use: zookeeper | ||
# persist-data: true | ||
# - use: kafka | ||
# persist-data: true |
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.
minor: remove if unused
@@ -652,6 +671,9 @@ template: | |||
# Minio instances used by this compute node | |||
provide-minio: "minio*" | |||
|
|||
# AWS s3 bucket used by this compute node |
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.
Should we provide correct documentation?
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.
Sorry I missed this typo, will fix it in next PR.
@@ -49,6 +49,12 @@ services: | |||
image: public.ecr.aws/x5u3w5h6/rw-build-env:v20230208_05 | |||
volumes: | |||
- ..:/risingwave | |||
|
|||
rw-build-env-hdfs: |
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.
Why do we need another image? It should be okay for us to integrate the hdfs environment in the default image, just like Kafka, etc.
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.
Currently our build image is about 2.7G, and adding hadoop
environment will raise it to 3.7G, which is really heavy. After some offline discussion, we decide to build an image with hadoop
only used in CI check, and do not include hdfs in e2e tests.
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.
IIUC, there were the steps of building the hdfs env in the Dockerfile
on Feb.9 in this PR, so we get an image that can be used for hdfs tests by specifying this image tag here. However, this seems to prevent us from updating the image in the future like bumping the sqllogictest version by specifying a fixed image tag here. cc @xxchan
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.
there were the steps of building the hdfs env in the
Dockerfile
on Feb.9 in this PR
Yes, that is to build the special hdfs image, and if we keep the steps of building the hdfs env in the Dockerfile
, every time someone change this Dockerfile
, it will build a new image with hdfs, but we only used in one step in pull request. 🤔
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
support hdfs by using
opendal
, and useopendal
memory engine for unit tests.Checklist
./risedev check
(or alias,./risedev c
)Documentation
If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.
Types of user-facing changes
Please keep the types that apply to your changes, and remove those that do not apply.
Release note
Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.
Refer to a related PR or issue link (optional)
part of #7310