-
Notifications
You must be signed in to change notification settings - Fork 591
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(connector): support local fs source #14312
Conversation
5e847e1
to
e02d209
Compare
The main-cron test of local_fs has passed. Check details here. |
// TODO(Kexiang): Currently, FsListnenr in opendal does not support a prefix. (Seems a bug in opendal) | ||
// So we assign prefix to empty string. |
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.
so we can deny the prefix in props?
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.
If this fs source is only used in testing, it's ok to ignore the miss of prefix list.
BTW, I'm not aware that OpenDAL Fs engine does not support prefix list. cc @Xuanwo
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.
The prefix
is not in props now. In other implemenataion, it's extracted from the match_pattern
(eg, the prefix of abc*.csv
is abc
) and used for filtering out files with a different prefix under the directory/bucket. Since Opendal does not support this listing with prefix
correctly, we simply set the prefix to empty. If the problem in opendal is solved, we can then generate the prefix here and return it, it benefits to performance.
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.
BTW, I'm not aware that OpenDAL Fs engine does not support prefix list. cc @Xuanwo
opendal 0.44 should fix it.
As I mentioned before, the prefix support for fs is simulated: we will list the parent dir and return the matched prefix. Since fs doesn't have prefix list support natively.
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.
Join the conversation on adding glob support to opendal – your input would be valuable: apache/opendal#3500
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.
The pr LGTM as long as it satisfies your testing requirements. also please some doc for all accepted props.
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.
LGTM as this change is only used for test.
// TODO(Kexiang): Currently, FsListnenr in opendal does not support a prefix. (Seems a bug in opendal) | ||
// So we assign prefix to empty string. |
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.
If this fs source is only used in testing, it's ok to ignore the miss of prefix list.
BTW, I'm not aware that OpenDAL Fs engine does not support prefix list. cc @Xuanwo
0bafe88
to
4b48e2a
Compare
4b48e2a
to
e649fae
Compare
Co-authored-by: KeXiangWang <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
As title. let keep it in stealth and only used for test.
Posix fs source should only be used for testing.
For a single-CN cluster, the behavior is well-defined. It will read from the local file system.
For a multi-CN cluster, each CN will read from its own local file system under the given directory.
There are two parameters needed:
An example:
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.