You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@seatunnel.apache.org by GitBox <gi...@apache.org> on 2022/04/21 12:49:12 UTC
[GitHub] [incubator-seatunnel] BenJFan opened a new pull request, #1722: [Feature] [Connector] Split connector jar from release core jar
BenJFan opened a new pull request, #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722
<!--
Thank you for contributing to SeaTunnel! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
## Contribution Checklist
- Make sure that the pull request corresponds to a [GITHUB issue](https://github.com/apache/incubator-seatunnel/issues).
- Name the pull request in the form "[Feature] [component] Title of the pull request", where *Feature* can be replaced by `Hotfix`, `Bug`, etc.
- Minor fixes should be named following this pattern: `[hotfix] [docs] Fix typo in README.md doc`.
-->
## Purpose of this pull request
1. Split connector jar from core jar in binary package
2. Support Spark/Flink submit job with multi jars
This close #1669
<!-- Describe the purpose of this pull request. For example: This pull request adds checkstyle plugin.-->
## Check list
* [ ] Code changed are covered with tests, or it does not need tests for reason:
* [ ] If any new Jar binary package adding in your PR, please add License Notice according
[New License Guide](https://github.com/apache/incubator-seatunnel/blob/dev/docs/en/contribution/new-license.md)
* [ ] If necessary, please update the documentation to describe the new feature. https://github.com/apache/incubator-seatunnel/tree/dev/docs
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858233535
##########
seatunnel-apis/seatunnel-api-spark/src/main/java/org/apache/seatunnel/spark/SparkEnvironment.java:
##########
@@ -81,6 +88,12 @@ public CheckResult checkConfig() {
return CheckResult.success();
}
+ @Override
+ public void registerPlugin(List<URL> pluginPaths) {
+ LOGGER.info("register plugins :" + pluginPaths);
+ // this.sparkSession.conf().set("spark.jars",pluginPaths.stream().map(URL::getPath).collect(Collectors.joining(",")));
Review Comment:
If so, it's need to add a `TODO` here.
##########
seatunnel-apis/seatunnel-api-spark/src/main/java/org/apache/seatunnel/spark/SparkEnvironment.java:
##########
@@ -81,6 +88,12 @@ public CheckResult checkConfig() {
return CheckResult.success();
}
+ @Override
+ public void registerPlugin(List<URL> pluginPaths) {
+ LOGGER.info("register plugins :" + pluginPaths);
+ // this.sparkSession.conf().set("spark.jars",pluginPaths.stream().map(URL::getPath).collect(Collectors.joining(",")));
Review Comment:
If so, it's needed to add a `TODO` here.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858203099
##########
seatunnel-apis/seatunnel-api-spark/src/main/java/org/apache/seatunnel/spark/SparkEnvironment.java:
##########
@@ -81,6 +88,12 @@ public CheckResult checkConfig() {
return CheckResult.success();
}
+ @Override
+ public void registerPlugin(List<URL> pluginPaths) {
+ LOGGER.info("register plugins :" + pluginPaths);
+ // this.sparkSession.conf().set("spark.jars",pluginPaths.stream().map(URL::getPath).collect(Collectors.joining(",")));
Review Comment:
Please remove the comment `//` here.
##########
seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/ReflectionUtils.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.seatunnel.common.utils;
+
+import java.lang.reflect.Method;
+
+public class ReflectionUtils {
+
+ public static Method getDeclaredMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
Review Comment:
Please use `Option<Method>` to this method result, this can help to know this method's result might be null.
##########
seatunnel-dist/release-docs/LICENSE:
##########
@@ -968,44 +1018,44 @@ CDDL License
The following components are provided under the CDDL License. See project link for details.
The text of each license is also included at licenses/LICENSE-[project].txt.
- (CDDL License) Expression Language 3.0 (org.glassfish:javax.el:3.0.0 - http://el-spec.java.net)
- (CDDL License) Expression Language 3.0 (org.glassfish:javax.el:3.0.1-b12 - http://uel.java.net)
- (CDDL License) Expression Language 3.0 API (javax.el:javax.el-api:3.0.0 - http://uel-spec.java.net)
- (CDDL License) HK2 API module (org.glassfish.hk2:hk2-api:2.4.0-b34 - https://hk2.java.net/hk2-api)
- (CDDL License) HK2 API module (org.glassfish.hk2:hk2-api:2.5.0-b32 - https://hk2.java.net/hk2-api)
- (CDDL License) HK2 Implementation Utilities (org.glassfish.hk2:hk2-utils:2.4.0-b34 - https://hk2.java.net/hk2-utils)
- (CDDL License) HK2 Implementation Utilities (org.glassfish.hk2:hk2-utils:2.5.0-b32 - https://hk2.java.net/hk2-utils)
- (CDDL License) JSP implementation (org.glassfish.web:javax.servlet.jsp:2.3.2 - http://jsp.java.net)
- (CDDL License) Java Servlet API (javax.servlet.jsp:jsp-api:2.1 - https://javaee.github.io/javaee-jsp-api)
- (CDDL License) Java Servlet API (javax.servlet:javax.servlet-api:3.1.0 - http://servlet-spec.java.net)
- (CDDL License) Java Servlet API (javax.servlet:servlet-api:2.5 - http://servlet-spec.java.net)
- (CDDL License) Java Transaction API (javax.transaction:jta:1.1 - http://java.sun.com/products/jta)
- (CDDL License) OSGi resource locator bundle - used by various API providers that rely on META-INF/services mechanism to locate providers. (org.glassfish.hk2:osgi-resource-locator:1.0.1 - http://glassfish.org/osgi-resource-locator/)
- (CDDL License) ServiceLocator Default Implementation (org.glassfish.hk2:hk2-locator:2.4.0-b34 - https://hk2.java.net/hk2-locator)
- (CDDL License) ServiceLocator Default Implementation (org.glassfish.hk2:hk2-locator:2.5.0-b32 - https://hk2.java.net/hk2-locator)
- (CDDL License) aopalliance version 1.0 repackaged as a module (org.glassfish.hk2.external:aopalliance-repackaged:2.4.0-b34 - https://hk2.java.net/external/aopalliance-repackaged)
- (CDDL License) aopalliance version 1.0 repackaged as a module (org.glassfish.hk2.external:aopalliance-repackaged:2.5.0-b32 - https://hk2.java.net/external/aopalliance-repackaged)
- (CDDL License) javax.annotation API (javax.annotation:javax.annotation-api:1.2 - http://jcp.org/en/jsr/detail?id=250)
- (CDDL License) javax.annotation API (javax.annotation:javax.annotation-api:1.3.2 - http://jcp.org/en/jsr/detail?id=250)
- (CDDL License) javax.inject:1 as OSGi bundle (org.glassfish.hk2.external:javax.inject:2.4.0-b34 - https://hk2.java.net/external/javax.inject)
- (CDDL License) javax.inject:1 as OSGi bundle (org.glassfish.hk2.external:javax.inject:2.5.0-b32 - https://hk2.java.net/external/javax.inject)
- (CDDL License) jsr311-api (javax.ws.rs:jsr311-api:1.1.1 - https://jsr311.dev.java.net)
- (CDDL License) jersey-container-servlet (org.glassfish.jersey.containers:jersey-container-servlet:2.22.2 - https://jersey.java.net/project/jersey-container-servlet/)
- (CDDL License) jersey-container-servlet-core (org.glassfish.jersey.containers:jersey-container-servlet-core:2.22.2 - https://jersey.java.net/project/jersey-container-servlet-core/)
- (CDDL License) jersey-container-servlet-core (org.glassfish.jersey.containers:jersey-container-servlet-core:2.25.1 - https://jersey.java.net/project/jersey-container-servlet-core/)
- (CDDL License) jersey-core-client (org.glassfish.jersey.core:jersey-client:2.22.2 - https://jersey.java.net/jersey-client/)
- (CDDL License) jersey-core-client (org.glassfish.jersey.core:jersey-client:2.25.1 - https://jersey.java.net/jersey-client/)
- (CDDL License) jersey-core-common (org.glassfish.jersey.core:jersey-common:2.22.2 - https://jersey.java.net/jersey-common/)
- (CDDL License) jersey-core-common (org.glassfish.jersey.core:jersey-common:2.25.1 - https://jersey.java.net/jersey-common/)
- (CDDL License) jersey-core-server (org.glassfish.jersey.core:jersey-server:2.22.2 - https://jersey.java.net/jersey-server/)
- (CDDL License) jersey-core-server (org.glassfish.jersey.core:jersey-server:2.25.1 - https://jersey.java.net/jersey-server/)
- (CDDL License) jersey-media-jaxb (org.glassfish.jersey.media:jersey-media-jaxb:2.22.2 - https://jersey.java.net/project/jersey-media-jaxb/)
- (CDDL License) jersey-media-jaxb (org.glassfish.jersey.media:jersey-media-jaxb:2.25.1 - https://jersey.java.net/project/jersey-media-jaxb/)
- (CDDL License) jersey-repackaged-guava (org.glassfish.jersey.bundles.repackaged:jersey-guava:2.22.2 - https://jersey.java.net/project/project/jersey-guava/)
- (CDDL License) jersey-repackaged-guava (org.glassfish.jersey.bundles.repackaged:jersey-guava:2.25.1 - https://jersey.java.net/project/project/jersey-guava/)
- (CDDL License) JavaBeans Activation Framework (com.sun.activation:javax.activation:1.2.0 - http://java.net/all/javax.activation/)
- (CDDL License) JavaBeans Activation Framework API jar (javax.activation:javax.activation-api:1.2.0 - http://java.net/all/javax.activation-api/)
- (CDDL License) JavaMail API (com.sun.mail:javax.mail:1.5.6 - http://javamail.java.net/javax.mail)
+ (CDDL + GPLv2 with classpath exception) Expression Language 3.0 (org.glassfish:javax.el:3.0.0 - http://el-spec.java.net)
Review Comment:
We shouldn't redeclare the duplicate license, please see #1644
##########
seatunnel-connectors/seatunnel-connectors-flink-list/pom.xml:
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <groupId>org.apache.seatunnel</groupId>
+ <artifactId>seatunnel-connectors</artifactId>
+ <version>${revision}</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>seatunnel-connectors-flink-list</artifactId>
+
+ <packaging>pom</packaging>
+
+ <modules>
+ <module>seatunnel-connectors-flink-list-current</module>
+ <module>seatunnel-connectors-flink-list-previous</module>
Review Comment:
Can we only use one Flink connector module to store all connectors? It seems we will not rely on the `seatunnel-flink-connectors` module in our project, so there will not occur class conflict.
In example module, we can directly set the target plugin module.
What I concern is that these two module is not clear, and it doesn't solve the conflict problem, if we need to support another connector version, we need to add another module `seatunnel-connectors-flink-list-xx`.
##########
seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/ReflectionUtils.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.seatunnel.common.utils;
+
+import java.lang.reflect.Method;
+
+public class ReflectionUtils {
+
+ public static Method getDeclaredMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
+
+ Method method;
+
+ for (; clazz != Object.class; clazz = clazz.getSuperclass()) {
+ try {
+ method = clazz.getDeclaredMethod(methodName, parameterTypes);
+ method.setAccessible(true);
+ return method;
+ } catch (Exception e) {
+ // do nothing
Review Comment:
I think we may need to log the exception here, if you think this exception is not important, we can just log in `warn` level.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858231365
##########
seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/ReflectionUtils.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.seatunnel.common.utils;
+
+import java.lang.reflect.Method;
+
+public class ReflectionUtils {
+
+ public static Method getDeclaredMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
+
+ Method method;
+
+ for (; clazz != Object.class; clazz = clazz.getSuperclass()) {
+ try {
+ method = clazz.getDeclaredMethod(methodName, parameterTypes);
+ method.setAccessible(true);
+ return method;
+ } catch (Exception e) {
+ // do nothing
Review Comment:
This logic is in the loop. Throw Execption is very normal thing. Because throw Execption so it can go on search method.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r855819992
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/config/PluginFactory.java:
##########
@@ -70,16 +82,62 @@
PLUGIN_BASE_CLASS_MAP.put(EngineType.FLINK, flinkBaseClassMap);
}
- public PluginFactory(Config config, EngineType engineType) {
+ public PluginFactory(Config config, EngineType engineType, String seaTunnelHome) {
this.config = config;
this.engineType = engineType;
+ this.seaTunnelHome = seaTunnelHome;
+ this.pluginJarPaths = searchPluginJar();
+ this.defaultClassLoader = initClassLoaderWithPaths(this.pluginJarPaths);
+ }
+
+ private ClassLoader initClassLoaderWithPaths(List<URL> pluginJarPaths) {
+ return new URLClassLoader(pluginJarPaths.toArray(new URL[0]),
+ Thread.currentThread().getContextClassLoader());
+ }
+
+ @Nonnull
+ private List<URL> searchPluginJar() {
+
+ File pluginDir = new File(this.seaTunnelHome + "/" + PLUGIN_DIR_NAME + "/" + this.engineType.getEngine());
+ if (!pluginDir.exists() || pluginDir.listFiles() == null) {
+ return new ArrayList<>();
+ }
+ File[] plugins =
+ Arrays.stream(pluginDir.listFiles()).filter(f -> f.getName().endsWith(".jar")).toArray(File[]::new);
+ return Arrays.stream(PluginType.values()).flatMap(type -> {
+ List<URL> pluginList = new ArrayList<>();
+ List<? extends Config> configList = config.getConfigList(type.getType());
+ configList.forEach(pluginConfig -> {
+ try {
+ for (File plugin : plugins) {
+ try {
+ createPluginInstanceIgnoreCase(type, pluginConfig.getString(PLUGIN_NAME_KEY),
+ new URLClassLoader(new URL[]{plugin.toURI().toURL()},
+ Thread.currentThread().getContextClassLoader()));
+ pluginList.add(plugin.toURI().toURL());
+ break;
+ } catch (ClassNotFoundException ignored) {
+ // do nothing
+ }
+
+ }
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ });
+ return pluginList.stream();
+ }).collect(Collectors.toList());
+ }
Review Comment:
> I don't think we need to load plugin class and create instance in this part. This part is just to find which jar need to be used in this job. We don't need to load all jars, if we load all class, there may still exist some class conflict issue. We already know the mapping of plugin to plugin jar, so we can easily to find the jar path.
> @BenJFan I still doubt that this way can work. Since we still use `CliFrontend` to submit our Flink job, so the main class defined in `SeatunnelFlink` will be executed in Flink master side, not in our client side. Don't we need to overwrite the `CliFrontend`?
At now, we use flink client mode, don't need overwrite it, but if we use cluster mode in the future, maybe we should consider about it.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1106033831
> > I don't review the whole code, it seems this PR is using the `--home` to add dependencies jars. So we need to install `SeaTunnel` at each JobMaster's `--home` directory? I am not sure if this way can run successful in yarn mode. BTW, please add doc about how to start SeaTunnel.
>
> Nothing change for user perception
Ok, I see the code, you add the connectors into `configuration`. If so, you shoundn't add `--home` in args, e.g. we use `plugin`directory, and we didn't add this in args.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858232240
##########
seatunnel-apis/seatunnel-api-spark/src/main/java/org/apache/seatunnel/spark/SparkEnvironment.java:
##########
@@ -81,6 +88,12 @@ public CheckResult checkConfig() {
return CheckResult.success();
}
+ @Override
+ public void registerPlugin(List<URL> pluginPaths) {
+ LOGGER.info("register plugins :" + pluginPaths);
+ // this.sparkSession.conf().set("spark.jars",pluginPaths.stream().map(URL::getPath).collect(Collectors.joining(",")));
Review Comment:
> Please remove the comment `//` here.
Because we use `--jar` parmeter, so registerPlugin will be empty at now.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858244661
##########
seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/ReflectionUtils.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.seatunnel.common.utils;
+
+import java.lang.reflect.Method;
+
+public class ReflectionUtils {
+
+ public static Method getDeclaredMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
+
+ Method method;
+
+ for (; clazz != Object.class; clazz = clazz.getSuperclass()) {
+ try {
+ method = clazz.getDeclaredMethod(methodName, parameterTypes);
+ method.setAccessible(true);
+ return method;
+ } catch (Exception e) {
+ // do nothing
Review Comment:
Ok, I see the code, when the method is not exist, there will throw the `NoSuchMethodException`, you need to catch it and find from parent, so for other exception, you don't eat it.
Suggest to change the implementation to below:
```java
@Nullable
public static Method findMethod(Class<?> clazz, String name, @Nullable Class<?>... paramTypes) {
Assert.notNull(clazz, "Class must not be null");
Assert.notNull(name, "Method name must not be null");
for(Class searchType = clazz; searchType != null; searchType = searchType.getSuperclass()) {
Method[] methods = searchType.isInterface() ? searchType.getMethods() : getDeclaredMethods(searchType, false);
Method[] var5 = methods;
int var6 = methods.length;
for(int var7 = 0; var7 < var6; ++var7) {
Method method = var5[var7];
if (name.equals(method.getName()) && (paramTypes == null || hasSameParams(method, paramTypes))) {
return method;
}
}
}
return null;
}
private static boolean hasSameParams(Method method, Class<?>[] paramTypes) {
return paramTypes.length == method.getParameterCount() && Arrays.equals(paramTypes, method.getParameterTypes());
}
```
The code is copy from spring :).
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1108563614
@ruanwenjun Hi, PTAL
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858248862
##########
seatunnel-connectors/seatunnel-connectors-flink-list/pom.xml:
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <groupId>org.apache.seatunnel</groupId>
+ <artifactId>seatunnel-connectors</artifactId>
+ <version>${revision}</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>seatunnel-connectors-flink-list</artifactId>
+
+ <packaging>pom</packaging>
+
+ <modules>
+ <module>seatunnel-connectors-flink-list-current</module>
+ <module>seatunnel-connectors-flink-list-previous</module>
Review Comment:
Ok, I just concern the module name will make user confuse.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1110989733
@CalvinKirs Please take a look.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1108021844
> Have some problem at now. @ruanwenjun @CalvinKirs Since flink only provides client mode now, we can let the driver identify which jars need to be uploaded to the cluster. But spark provides cluster mode, our driver needs to run on the cluster, so we can't configure the jar package that needs to be submitted to the cluster through code, because the driver code runs on the cluster. Spark provides the --jar parameter to configure the jar package submitted to the cluster. So now we need to identify which jar packages need to be provided to the cluster according to config, and we need to complete the identification of jar packages in `SparkStarter`. But now the jar package identification is achieved by loading the jar package through the classloader and calling the `getPluginName` method to achieve matching. The `seatunnel-core-spark.jar` corresponding to `SparkStarter` does not provide the dependency corresponding to spark, so the jar package cannot be loaded successfully.
>
> Is there a good solution for this?
>
> 1. Add Spark dependency to `seatunnel-core-spark.jar`
> 2. Separate a module to do `SparkStarter` related things, and add Spark dependencies at the same time
> 3. Change a jar package identification method
I suggest that we can maintain a plugin mapping file in conf directory. The content looks like below
```properties
Clickhouse=seatunnel-connector-spark-clickhouse.jar
ClickhouseFile=seatunnel-connector-spark-clickhouse.jar
Console=seatunnel-connector-spark-console.jar
```
Then we can easily find out which plugin jar should be used, and once we add a new plugin, we need to add a mapping in this file.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858248153
##########
seatunnel-connectors/seatunnel-connectors-flink-list/pom.xml:
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <groupId>org.apache.seatunnel</groupId>
+ <artifactId>seatunnel-connectors</artifactId>
+ <version>${revision}</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>seatunnel-connectors-flink-list</artifactId>
+
+ <packaging>pom</packaging>
+
+ <modules>
+ <module>seatunnel-connectors-flink-list-current</module>
+ <module>seatunnel-connectors-flink-list-previous</module>
Review Comment:
Yes, but the starter will search jar from directory, the two different module to identify which directory( connectors or opt ) the connector jar should move to.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858246898
##########
seatunnel-connectors/seatunnel-connectors-flink-list/pom.xml:
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <groupId>org.apache.seatunnel</groupId>
+ <artifactId>seatunnel-connectors</artifactId>
+ <version>${revision}</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>seatunnel-connectors-flink-list</artifactId>
+
+ <packaging>pom</packaging>
+
+ <modules>
+ <module>seatunnel-connectors-flink-list-current</module>
+ <module>seatunnel-connectors-flink-list-previous</module>
Review Comment:
In my knowledge, the starter will not need to rely on the connector module after your change.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun merged pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun merged PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858302573
##########
seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/ReflectionUtils.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.seatunnel.common.utils;
+
+import java.lang.reflect.Method;
+
+public class ReflectionUtils {
+
+ public static Method getDeclaredMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
+
+ Method method;
+
+ for (; clazz != Object.class; clazz = clazz.getSuperclass()) {
+ try {
+ method = clazz.getDeclaredMethod(methodName, parameterTypes);
+ method.setAccessible(true);
+ return method;
+ } catch (Exception e) {
+ // do nothing
Review Comment:
I am not sure if catch `Exception` here is OK, since the `getMethod` will throw two checkedException `NoSuchMethodException` and `SecurityException`, ignore all exception is not a good way.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858249238
##########
seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/ReflectionUtils.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.seatunnel.common.utils;
+
+import java.lang.reflect.Method;
+
+public class ReflectionUtils {
+
+ public static Method getDeclaredMethod(Class<?> clazz, String methodName, Class<?>... parameterTypes) {
+
+ Method method;
+
+ for (; clazz != Object.class; clazz = clazz.getSuperclass()) {
+ try {
+ method = clazz.getDeclaredMethod(methodName, parameterTypes);
+ method.setAccessible(true);
+ return method;
+ } catch (Exception e) {
+ // do nothing
Review Comment:
Seem like Spring's reflect method is complex than ours. But if we method is enough, I think maybe change it when when need change it.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1105955095
> I don't review the whole code, it seems this PR is using the `--home` to add dependencies jars. So we need to install `SeaTunnel` at each JobMaster's `--home` directory? I am not sure if this way can run successful in yarn mode. BTW, please add doc about how to start SeaTunnel.
Nothing change for user perception
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858249816
##########
seatunnel-connectors/seatunnel-connectors-flink-list/pom.xml:
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <groupId>org.apache.seatunnel</groupId>
+ <artifactId>seatunnel-connectors</artifactId>
+ <version>${revision}</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>seatunnel-connectors-flink-list</artifactId>
+
+ <packaging>pom</packaging>
+
+ <modules>
+ <module>seatunnel-connectors-flink-list-current</module>
+ <module>seatunnel-connectors-flink-list-previous</module>
Review Comment:
> Ok, I just concern the module name will make user confuse.
We will add doc to developer start.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1108028351
> > Have some problem at now. @ruanwenjun @CalvinKirs Since flink only provides client mode now, we can let the driver identify which jars need to be uploaded to the cluster. But spark provides cluster mode, our driver needs to run on the cluster, so we can't configure the jar package that needs to be submitted to the cluster through code, because the driver code runs on the cluster. Spark provides the --jar parameter to configure the jar package submitted to the cluster. So now we need to identify which jar packages need to be provided to the cluster according to config, and we need to complete the identification of jar packages in `SparkStarter`. But now the jar package identification is achieved by loading the jar package through the classloader and calling the `getPluginName` method to achieve matching. The `seatunnel-core-spark.jar` corresponding to `SparkStarter` does not provide the dependency corresponding to spark, so the jar package cannot be loaded successfully.
> > Is there a good solution for this?
> >
> > 1. Add Spark dependency to `seatunnel-core-spark.jar`
> > 2. Separate a module to do `SparkStarter` related things, and add Spark dependencies at the same time
> > 3. Change a jar package identification method
>
> I suggest that we can maintain a plugin mapping file in conf directory. The content looks like below
>
> ```ini
> Clickhouse=seatunnel-connector-spark-clickhouse.jar
> ClickhouseFile=seatunnel-connector-spark-clickhouse.jar
> Console=seatunnel-connector-spark-console.jar
> ```
>
> Then we can easily find out which plugin jar should be used, and once we add a new plugin, we need to add a mapping in this file.
This is a good idea, I will finish it.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r858238682
##########
seatunnel-connectors/seatunnel-connectors-flink-list/pom.xml:
##########
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ ~ 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.
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <parent>
+ <groupId>org.apache.seatunnel</groupId>
+ <artifactId>seatunnel-connectors</artifactId>
+ <version>${revision}</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>seatunnel-connectors-flink-list</artifactId>
+
+ <packaging>pom</packaging>
+
+ <modules>
+ <module>seatunnel-connectors-flink-list-current</module>
+ <module>seatunnel-connectors-flink-list-previous</module>
Review Comment:
This constructure to split two type connector: new version and old version. So we can move it to two different directory. Then starter will only to load jar in new version directory. In example module to add list-current as dependency just make user run easier
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1105953947
> I don't review the whole code, it seems this PR is using the `--home` to add dependencies jars. So we need to install `SeaTunnel` at each JobMaster's `--home` directory? I am not sure if this way can run successful in yarn mode. BTW, please add doc about how to start SeaTunnel.
The `--home` is option to config seatunnel home. So program will know where is connector jar location. At now, user don't change any shell command to start SeaTunnel, same with before.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1105948762
I don't review the whole code, it seems this PR is using the `--home` to add dependencies jars. So we need to install `SeaTunnel` at each JobMaster's `--home` directory? I am not sure if this way can run successful in yarn mode.
BTW, please add doc about how to start SeaTunnel.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1106049816
> > > I don't review the whole code, it seems this PR is using the `--home` to add dependencies jars. So we need to install `SeaTunnel` at each JobMaster's `--home` directory? I am not sure if this way can run successful in yarn mode. BTW, please add doc about how to start SeaTunnel.
> >
> >
> > Nothing change for user perception
>
> Ok, I see the code, you add the connectors into `configuration`. If so, you shoundn't add `--home` in args, e.g. we use `plugin`directory, and we didn't add this in args.
The message is very helpful, I will change it.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] BenJFan commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
BenJFan commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1107778679
Have some problem at now. @ruanwenjun @CalvinKirs
Since flink only provides client mode now, we can let the driver identify which jars need to be uploaded to the cluster. But spark provides cluster mode, our driver needs to run on the cluster, so we can't configure the jar package that needs to be submitted to the cluster through code, because the driver code runs on the cluster. Spark provides the --jar parameter to configure the jar package submitted to the cluster. So now we need to identify which jar packages need to be provided to the cluster according to config, and we need to complete the identification of jar packages in `SparkStarter`. But now the jar package identification is achieved by loading the jar package through the classloader and calling the `getPluginName` method to achieve matching. The `seatunnel-core-spark.jar` corresponding to `SparkStarter` does not provide the dependency corresponding to spark, so the jar package cannot be loaded successfully.
Is there a good solution for this?
1. Add Spark dependency to `seatunnel-core-spark.jar`
2. Separate a module to do `SparkStarter` related things, and add Spark dependencies at the same time
3. Change a jar package identification method
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a diff in pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#discussion_r855793429
##########
seatunnel-core/seatunnel-core-base/src/main/java/org/apache/seatunnel/config/PluginFactory.java:
##########
@@ -70,16 +82,62 @@
PLUGIN_BASE_CLASS_MAP.put(EngineType.FLINK, flinkBaseClassMap);
}
- public PluginFactory(Config config, EngineType engineType) {
+ public PluginFactory(Config config, EngineType engineType, String seaTunnelHome) {
this.config = config;
this.engineType = engineType;
+ this.seaTunnelHome = seaTunnelHome;
+ this.pluginJarPaths = searchPluginJar();
+ this.defaultClassLoader = initClassLoaderWithPaths(this.pluginJarPaths);
+ }
+
+ private ClassLoader initClassLoaderWithPaths(List<URL> pluginJarPaths) {
+ return new URLClassLoader(pluginJarPaths.toArray(new URL[0]),
+ Thread.currentThread().getContextClassLoader());
+ }
+
+ @Nonnull
+ private List<URL> searchPluginJar() {
+
+ File pluginDir = new File(this.seaTunnelHome + "/" + PLUGIN_DIR_NAME + "/" + this.engineType.getEngine());
+ if (!pluginDir.exists() || pluginDir.listFiles() == null) {
+ return new ArrayList<>();
+ }
+ File[] plugins =
+ Arrays.stream(pluginDir.listFiles()).filter(f -> f.getName().endsWith(".jar")).toArray(File[]::new);
+ return Arrays.stream(PluginType.values()).flatMap(type -> {
+ List<URL> pluginList = new ArrayList<>();
+ List<? extends Config> configList = config.getConfigList(type.getType());
+ configList.forEach(pluginConfig -> {
+ try {
+ for (File plugin : plugins) {
+ try {
+ createPluginInstanceIgnoreCase(type, pluginConfig.getString(PLUGIN_NAME_KEY),
+ new URLClassLoader(new URL[]{plugin.toURI().toURL()},
+ Thread.currentThread().getContextClassLoader()));
+ pluginList.add(plugin.toURI().toURL());
+ break;
+ } catch (ClassNotFoundException ignored) {
+ // do nothing
+ }
+
+ }
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ });
+ return pluginList.stream();
+ }).collect(Collectors.toList());
+ }
Review Comment:
I don't think we need to load plugin class and create instance in this part.
This part is just to find which jar need to be used in this job. We don't need to load all jars, if we load all class, there may still exist some class conflict issue. We already know the mapping of plugin to plugin jar, so we can easily to find the jar path.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1722: [Feature] [Connector] Split connector jar from release core jar
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on PR #1722:
URL: https://github.com/apache/incubator-seatunnel/pull/1722#issuecomment-1106047494
@BenJFan I still doubt that this way can work. Since we still use `CliFrontend` to submit our Flink job, so the main class defined in `SeatunnelFlink` will be executed in Flink master side, not in our client side.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org