-
Notifications
You must be signed in to change notification settings - Fork 470
[File system] Add Google Cloud Storage integration #631
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
Conversation
|
Will add integration tests. Putting temporary to draft |
|
Added an integration test based on Since the service account key is in a file form and not in a string form I see two options.
In the pr I followed the second approach. |
|
@luoyuxia could you help to review it? |
|
@gkatzioura , could you check how Flink tests the gs-filesystem in CI? |
|
Hi @wuchong , after checking there are not tests for the actual filesystem integration. |
|
Thank you @gkatzioura , please ping me when you have finished the mock server. |
|
Hi @wuchong & @luoyuxia |
|
checkstyle are failed |
|
@luoyuxia could you help to review this PR? |
Sure, now I got some free time. I'll help review these days. |
luoyuxia
left a comment
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.
@gkatzioura Thanks for the greate work! Left some comments.
fluss-filesystems/fluss-fs-gs/src/main/java/com/alibaba/fluss/fs/gs/GSAFileSystemPlugin.java
Outdated
Show resolved
Hide resolved
fluss-filesystems/fluss-fs-gs/src/main/java/com/alibaba/fluss/fs/gs/GSFileSystem.java
Show resolved
Hide resolved
fluss-filesystems/fluss-fs-gs/src/main/java/com/alibaba/fluss/fs/gs/GSFileSystemPlugin.java
Outdated
Show resolved
Hide resolved
fluss-filesystems/fluss-fs-gs/src/test/java/com/alibaba/fluss/fs/gs/GSPathServerHandler.java
Outdated
Show resolved
Hide resolved
fluss-filesystems/fluss-fs-gs/src/test/java/com/alibaba/fluss/fs/gs/GSTestCredentials.java
Outdated
Show resolved
Hide resolved
fluss-filesystems/fluss-fs-gs/src/test/resources/fake-service-account.json
Show resolved
Hide resolved
fluss-filesystems/fluss-fs-gs/src/test/resources/fake-service-account.json
Show resolved
Hide resolved
|
Hi @gkatzioura , you can "request review" again when you have addressed all the comments, so that the reviewer can get the notification in time. |
luoyuxia
left a comment
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.
@gkatzioura Could you please rebase main branch and add licencese since currently licences check is enabeld.
...stems/fluss-fs-gs/src/main/resources/META-INF/services/com.alibaba.fluss.fs.FileSystemPlugin
Outdated
Show resolved
Hide resolved
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; | ||
|
|
||
| class GSFileSystemPluginTest { |
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 don't forget this comment.
694a42f to
c465149
Compare
|
@gkatzioura Thanks for the great work. I'll have a final check. Hope to merge in later days |
luoyuxia
left a comment
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.
@gkatzioura Thanks for the work. LGTM! I append a commit to add licenses and refine test. Please take a look. If you have no further comments, I'll merge later
0b8f285 to
bec6304
Compare
Purpose
Linked issue: close #630
Brief change log
Added the GSFileSystem based on the given Hadoop Google Cloud Storage file system
Tests
API and Format
No
Documentation
It adds support for the Google Cloud Storage Filesystem