You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@streampark.apache.org by GitBox <gi...@apache.org> on 2022/09/12 09:27:44 UTC
[GitHub] [incubator-streampark] legendtkl opened a new pull request, #1579: [Feature] implement Oss Storeage Service
legendtkl opened a new pull request, #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579
### Purpose of this pull request
We will support several remote storage services in StreamPark, and these storage services will be used in Flink/Spark on k8s for pod template as an artifacts upload/fetch.
This PR implement Aliyun OSS supports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] legendtkl commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968456094
##########
streampark-storage/src/main/java/org/apache/streampark/storage/StorageService.java:
##########
@@ -17,8 +17,11 @@
package org.apache.streampark.storage;
+/**
+ * StorageService will be used as artifacts fetcher in pod template, so it should rely on other modules.
+ */
public interface StorageService {
Review Comment:
> StorageService just be used as artifacts fetcher in pod template, right?
>
> If yes, the class name is unreasonable. The StorageService looks like a generic storage service.
@1996fanrui No, this will be used to uploading job-related resources to remote storage system.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] wolfboys merged pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
wolfboys merged PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] wolfboys commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
wolfboys commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968525571
##########
pom.xml:
##########
@@ -535,6 +538,24 @@
</dependency>
<!--log4j end-->
+ <dependency>
+ <groupId>org.projectlombok</groupId>
+ <artifactId>lombok</artifactId>
+ <version>${lombok.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>com.aliyun.oss</groupId>
+ <artifactId>aliyun-sdk-oss</artifactId>
+ <version>${aliyun.oss.version}</version>
+ </dependency>
Review Comment:
> I see you just move properties to `streampark-storage` module.
>
> As I understand, @wolfboys means move these dependencies to `streampark-storage` module, not just move properties. Root pom shouldn't add these dependencies, they should be added in `streampark-storage` module.
yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#issuecomment-1243818511
Hi @legendtkl , I still have some questions about these 2 comments, could you un-resolve them? thanks a lot.
https://github.com/apache/incubator-streampark/pull/1579#discussion_r968448518
https://github.com/apache/incubator-streampark/pull/1579#discussion_r968437610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] legendtkl commented on pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
legendtkl commented on PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#issuecomment-1244754117
> Hi @legendtkl , root pom just defined global general dependencies, e.g: scala, jackson, log4j, many modules used. aliyun-sdk-oss only used in streampark-storeage module, so, aliyun-sdk-oss dependency in the root POM should be deleted. and defined in streampark-storeage module. e.g: streampark-console use a lot of spring dependencies, but they are not defined in the root POM, defined in streampark-consoel module.
>
> root pom只用来定义全局通用的依赖, 例如: scala,jackson,log4j等, 很多模块都使用到了, 但是aliyun-sdk-oss这个只在streampark-storeage中被使用,是某个模块特有的依赖,不应该定义到root pom中,应该从root pom中删除,定义到具体的模块中, 如: 在streampark-console中,大量使用了spring的依赖,但是他们并没有被定义到root pom中,只是定义在streampark-consoel 模块中.
@wolfboys resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] wolfboys commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
wolfboys commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968251171
##########
pom.xml:
##########
@@ -535,6 +538,24 @@
</dependency>
<!--log4j end-->
+ <dependency>
+ <groupId>org.projectlombok</groupId>
+ <artifactId>lombok</artifactId>
+ <version>${lombok.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>com.aliyun.oss</groupId>
+ <artifactId>aliyun-sdk-oss</artifactId>
+ <version>${aliyun.oss.version}</version>
+ </dependency>
Review Comment:
The root pom declare global general dependencies, such as oss, s3, etc., which are the dependencies that only need to be used by streampark-storeage module. so, it is recommended to define it in streampark-storeage module
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968337671
##########
streampark-storage/src/main/java/org/apache/streampark/storage/oss/OssStorageService.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.streampark.storage.oss;
+
+import org.apache.streampark.storage.StorageService;
+
+import com.aliyun.oss.ClientException;
+import com.aliyun.oss.OSS;
+import com.aliyun.oss.OSSClientBuilder;
+import com.aliyun.oss.OSSException;
+import com.aliyun.oss.model.GetObjectRequest;
+import com.aliyun.oss.model.PutObjectRequest;
+import com.google.common.annotations.VisibleForTesting;
+import lombok.extern.slf4j.Slf4j;
+
+import java.io.File;
+
+@Slf4j
Review Comment:
The `@Slf4j` isn't used, right?
##########
streampark-storage/src/main/java/org/apache/streampark/storage/StorageService.java:
##########
@@ -17,8 +17,11 @@
package org.apache.streampark.storage;
+/**
+ * StorageService will be used as artifacts fetcher in pod template, so it should rely on other modules.
+ */
public interface StorageService {
Review Comment:
StorageService just be used as artifacts fetcher in pod template, right?
If yes, the class name is unreasonable. The StorageService looks like a generic storage service.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] legendtkl commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968369514
##########
pom.xml:
##########
@@ -535,6 +538,24 @@
</dependency>
<!--log4j end-->
+ <dependency>
+ <groupId>org.projectlombok</groupId>
+ <artifactId>lombok</artifactId>
+ <version>${lombok.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>com.aliyun.oss</groupId>
+ <artifactId>aliyun-sdk-oss</artifactId>
+ <version>${aliyun.oss.version}</version>
+ </dependency>
Review Comment:
addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968480676
##########
streampark-storage/src/main/java/org/apache/streampark/storage/StorageService.java:
##########
@@ -17,8 +17,11 @@
package org.apache.streampark.storage;
+/**
+ * StorageService will be used as artifacts fetcher in pod template, so it should rely on other modules.
+ */
public interface StorageService {
Review Comment:
Thanks for you quick reply.
If no, I have some questions:
1. why did you add the comments for `StorageService`?
> StorageService will be used as artifacts fetcher in pod template, so it should rely on other modules.
2. Whether the localFilePath parameter of 2 methods are reasonable?
- I worry that the current interface definition only solves the current needs and will not be scalable in the future.
- For example, will the distributed file system be supported in the future? Or just download the data into memory.
- Would it be more general to replace `String localFilePath` with `OutputStream/InputStream`?
3. Whether the objectPath of 2 methods are reasonable?
- As I understand, ObjectStorage is just one type of StorageService, it shouldn't always be `String objectPath`, right?
Please correct me if there are any mistakes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] legendtkl commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
legendtkl commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968454606
##########
streampark-storage/src/main/java/org/apache/streampark/storage/oss/OssStorageService.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.streampark.storage.oss;
+
+import org.apache.streampark.storage.StorageService;
+
+import com.aliyun.oss.ClientException;
+import com.aliyun.oss.OSS;
+import com.aliyun.oss.OSSClientBuilder;
+import com.aliyun.oss.OSSException;
+import com.aliyun.oss.model.GetObjectRequest;
+import com.aliyun.oss.model.PutObjectRequest;
+import com.google.common.annotations.VisibleForTesting;
+import lombok.extern.slf4j.Slf4j;
+
+import java.io.File;
+
+@Slf4j
Review Comment:
@1996fanrui Good catch. I will update the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on a diff in pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#discussion_r968448518
##########
pom.xml:
##########
@@ -535,6 +538,24 @@
</dependency>
<!--log4j end-->
+ <dependency>
+ <groupId>org.projectlombok</groupId>
+ <artifactId>lombok</artifactId>
+ <version>${lombok.version}</version>
+ </dependency>
+
+ <dependency>
+ <groupId>com.aliyun.oss</groupId>
+ <artifactId>aliyun-sdk-oss</artifactId>
+ <version>${aliyun.oss.version}</version>
+ </dependency>
Review Comment:
I see you just move properties to `streampark-storage` module.
As I understand, @wolfboys means move these dependencies to `streampark-storage` module, not just move properties. Root pom shouldn't add these dependencies, they should be added in `streampark-storage` module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] wolfboys commented on pull request #1579: [Feature] implement Oss Storeage Service
Posted by GitBox <gi...@apache.org>.
wolfboys commented on PR #1579:
URL: https://github.com/apache/incubator-streampark/pull/1579#issuecomment-1243864516
Hi @legendtkl , root pom just defined global general dependencies, e.g: scala, jackson, log4j, many modules used. aliyun-sdk-oss only used in streampark-storeage module, so, aliyun-sdk-oss dependency in the root POM should be deleted. and defined in streampark-storeage module.
e.g: streampark-console use a lot of spring dependencies, but they are not defined in the root POM, defined in streampark-consoel module.
-----
root pom只用来定义全局通用的依赖, 例如: scala,jackson,log4j等, 很多模块都使用到了, 但是aliyun-sdk-oss这个只在streampark-storeage中被使用,是某个模块特有的依赖,不应该定义到root pom中,应该从root pom中删除,定义到具体的模块中, 如: 在streampark-console中,大量使用了spring的依赖,但是他们并没有被定义到root pom中,只是定义在streampark-consoel 模块中.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org