You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@submarine.apache.org by GitBox <gi...@apache.org> on 2020/04/17 17:40:30 UTC
[GitHub] [submarine] jiwq opened a new pull request #266: SUBMARINE-467.
Simplify the submitter configuration
jiwq opened a new pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266
### What is this PR for?
At now the submarine server supports config more submitters, but it's not necessary in current stage. At the same time it also cause trouble to users. So we'd better simplify the configuration to keep only one submitter in server. Considering the project vision, we set the K8s submitter as the default.
### What type of PR is it?
[Improvement]
### Todos
### What is the Jira issue?
https://issues.apache.org/jira/browse/SUBMARINE-467
### How should this be tested?
https://travis-ci.com/github/jiwq/submarine/builds/160762675
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] liuxunorg commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
liuxunorg commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410695903
##########
File path: submarine-commons/commons-utils/src/main/java/org/apache/submarine/commons/utils/SubmarineConfVars.java
##########
@@ -68,10 +68,7 @@
WORKBENCH_WEB_WAR("workbench.web.war", "submarine-workbench/workbench-web/dist"),
SUBMARINE_RUNTIME_CLASS("submarine.runtime.class",
"org.apache.submarine.server.submitter.yarn.YarnRuntimeFactory"),
- SUBMARINE_SUBMITTERS("submarine.submitters", ""),
- SUBMARINE_SUBMITTERS_CLASS("submarine.submitters.%s.class", ""),
- SUBMARINE_SUBMITTERS_CLASSPATH("submarine.submitters.%s.classpath", ""),
- SUBMARINE_K8S_KUBE_CONFIG("submarine.k8s.kube.config", "");
+ SUBMARINE_SUBMITTER("submarine.submitter", "");
Review comment:
Please set the default value to k8s
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] jiwq commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
jiwq commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410715584
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -92,11 +98,13 @@ private void loadSubmitters() {
return urls.toArray(new URL[0]);
}
- /**
- * Get the specified submitter by submitter type
- * @return submitter
- */
- public JobSubmitter getSubmitterByType(String type) {
- return submitterMap.get(type);
+ private static class JobSubmitterConfig {
+ private final String clazzName;
+ private final String classpath;
+
+ JobSubmitterConfig(String clazzName, String classpath) {
+ this.clazzName = clazzName;
+ this.classpath = classpath;
Review comment:
The [classpath](https://en.wikipedia.org/wiki/Classpath_(Java)) is a parameter in JVM, so I think we should keep this naming convention.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] jiwq commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
jiwq commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410713586
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -30,44 +31,49 @@
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
+import java.util.Map;
/**
- * Submitter Manager, load all the submitter plugin, configured by
- * {@link SubmarineConfiguration.ConfVars#SUBMARINE_SUBMITTERS} and related key.
+ * Submitter Manager is responsible for load {@link JobSubmitter} implements class.
*/
public class SubmitterManager {
private static final Logger LOG = LoggerFactory.getLogger(SubmitterManager.class);
- private SubmarineConfiguration conf;
- private ConcurrentMap<String, JobSubmitter> submitterMap = new ConcurrentHashMap<>();
+ private static final SubmitterManager INSTANCE = new SubmitterManager();
- public SubmitterManager(SubmarineConfiguration conf) {
- this.conf = conf;
- loadSubmitters();
+ private final Map<String, JobSubmitterConfig> SUBMITTER_TABLE = new HashMap<>();
Review comment:
Good point, I had fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] jiwq commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
jiwq commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410715205
##########
File path: submarine-commons/commons-utils/src/main/java/org/apache/submarine/commons/utils/SubmarineConfVars.java
##########
@@ -68,10 +68,7 @@
WORKBENCH_WEB_WAR("workbench.web.war", "submarine-workbench/workbench-web/dist"),
SUBMARINE_RUNTIME_CLASS("submarine.runtime.class",
"org.apache.submarine.server.submitter.yarn.YarnRuntimeFactory"),
- SUBMARINE_SUBMITTERS("submarine.submitters", ""),
- SUBMARINE_SUBMITTERS_CLASS("submarine.submitters.%s.class", ""),
- SUBMARINE_SUBMITTERS_CLASSPATH("submarine.submitters.%s.classpath", ""),
- SUBMARINE_K8S_KUBE_CONFIG("submarine.k8s.kube.config", "");
+ SUBMARINE_SUBMITTER("submarine.submitter", "");
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] jiwq commented on issue #266: SUBMARINE-467. Simplify
the submitter configuration
Posted by GitBox <gi...@apache.org>.
jiwq commented on issue #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#issuecomment-615894881
Thanks @yuanzac and @liuxunorg help to review.
I read your opinions and answered each question. If I understand error, please correct me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] liuxunorg commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
liuxunorg commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410696463
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -92,11 +98,13 @@ private void loadSubmitters() {
return urls.toArray(new URL[0]);
}
- /**
- * Get the specified submitter by submitter type
- * @return submitter
- */
- public JobSubmitter getSubmitterByType(String type) {
- return submitterMap.get(type);
+ private static class JobSubmitterConfig {
+ private final String clazzName;
+ private final String classpath;
Review comment:
Please change `classpath` to `classPath`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] liuxunorg commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
liuxunorg commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410696306
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -30,44 +31,49 @@
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
+import java.util.Map;
/**
- * Submitter Manager, load all the submitter plugin, configured by
- * {@link SubmarineConfiguration.ConfVars#SUBMARINE_SUBMITTERS} and related key.
+ * Submitter Manager is responsible for load {@link JobSubmitter} implements class.
*/
public class SubmitterManager {
private static final Logger LOG = LoggerFactory.getLogger(SubmitterManager.class);
- private SubmarineConfiguration conf;
- private ConcurrentMap<String, JobSubmitter> submitterMap = new ConcurrentHashMap<>();
+ private static final SubmitterManager INSTANCE = new SubmitterManager();
- public SubmitterManager(SubmarineConfiguration conf) {
- this.conf = conf;
- loadSubmitters();
+ private final Map<String, JobSubmitterConfig> SUBMITTER_TABLE = new HashMap<>();
Review comment:
yes, The name SUBMITTER_TABLE is too strange.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] jiwq commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
jiwq commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410715570
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -92,11 +98,13 @@ private void loadSubmitters() {
return urls.toArray(new URL[0]);
}
- /**
- * Get the specified submitter by submitter type
- * @return submitter
- */
- public JobSubmitter getSubmitterByType(String type) {
- return submitterMap.get(type);
+ private static class JobSubmitterConfig {
+ private final String clazzName;
+ private final String classpath;
Review comment:
The [classpath](https://en.wikipedia.org/wiki/Classpath_(Java)) is a parameter in JVM, so I think we should keep this naming convention.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] yuanzac commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
yuanzac commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410678575
##########
File path: submarine-commons/commons-utils/src/main/java/org/apache/submarine/commons/utils/SubmarineConfVars.java
##########
@@ -68,10 +68,7 @@
WORKBENCH_WEB_WAR("workbench.web.war", "submarine-workbench/workbench-web/dist"),
SUBMARINE_RUNTIME_CLASS("submarine.runtime.class",
"org.apache.submarine.server.submitter.yarn.YarnRuntimeFactory"),
- SUBMARINE_SUBMITTERS("submarine.submitters", ""),
- SUBMARINE_SUBMITTERS_CLASS("submarine.submitters.%s.class", ""),
- SUBMARINE_SUBMITTERS_CLASSPATH("submarine.submitters.%s.classpath", ""),
- SUBMARINE_K8S_KUBE_CONFIG("submarine.k8s.kube.config", "");
+ SUBMARINE_SUBMITTER("submarine.submitter", "");
Review comment:
How about we make the fault value of SUBMARINE_SUBMITTER to k8s.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] liuxunorg commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
liuxunorg commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410696522
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -92,11 +98,13 @@ private void loadSubmitters() {
return urls.toArray(new URL[0]);
}
- /**
- * Get the specified submitter by submitter type
- * @return submitter
- */
- public JobSubmitter getSubmitterByType(String type) {
- return submitterMap.get(type);
+ private static class JobSubmitterConfig {
+ private final String clazzName;
+ private final String classpath;
+
+ JobSubmitterConfig(String clazzName, String classpath) {
+ this.clazzName = clazzName;
+ this.classpath = classpath;
Review comment:
Please change `classpath` to `classPath`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] yuanzac commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
yuanzac commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410678068
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -30,44 +31,49 @@
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
+import java.util.Map;
/**
- * Submitter Manager, load all the submitter plugin, configured by
- * {@link SubmarineConfiguration.ConfVars#SUBMARINE_SUBMITTERS} and related key.
+ * Submitter Manager is responsible for load {@link JobSubmitter} implements class.
*/
public class SubmitterManager {
private static final Logger LOG = LoggerFactory.getLogger(SubmitterManager.class);
- private SubmarineConfiguration conf;
- private ConcurrentMap<String, JobSubmitter> submitterMap = new ConcurrentHashMap<>();
+ private static final SubmitterManager INSTANCE = new SubmitterManager();
- public SubmitterManager(SubmarineConfiguration conf) {
- this.conf = conf;
- loadSubmitters();
+ private final Map<String, JobSubmitterConfig> SUBMITTER_TABLE = new HashMap<>();
Review comment:
We'd better rename SUBMITTER_TABLE to something like submitter_class_config.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] yuanzac commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
yuanzac commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410678192
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -30,44 +31,49 @@
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
+import java.util.Map;
/**
- * Submitter Manager, load all the submitter plugin, configured by
- * {@link SubmarineConfiguration.ConfVars#SUBMARINE_SUBMITTERS} and related key.
+ * Submitter Manager is responsible for load {@link JobSubmitter} implements class.
*/
public class SubmitterManager {
private static final Logger LOG = LoggerFactory.getLogger(SubmitterManager.class);
- private SubmarineConfiguration conf;
- private ConcurrentMap<String, JobSubmitter> submitterMap = new ConcurrentHashMap<>();
+ private static final SubmitterManager INSTANCE = new SubmitterManager();
- public SubmitterManager(SubmarineConfiguration conf) {
- this.conf = conf;
- loadSubmitters();
+ private final Map<String, JobSubmitterConfig> SUBMITTER_TABLE = new HashMap<>();
+
+ private JobSubmitter submitter;
+
+ {
Review comment:
We'd better move the initialization logic to a constructor 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [submarine] jiwq commented on a change in pull request #266:
SUBMARINE-467. Simplify the submitter configuration
Posted by GitBox <gi...@apache.org>.
jiwq commented on a change in pull request #266: SUBMARINE-467. Simplify the submitter configuration
URL: https://github.com/apache/submarine/pull/266#discussion_r410715143
##########
File path: submarine-server/server-core/src/main/java/org/apache/submarine/server/SubmitterManager.java
##########
@@ -30,44 +31,49 @@
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
+import java.util.Map;
/**
- * Submitter Manager, load all the submitter plugin, configured by
- * {@link SubmarineConfiguration.ConfVars#SUBMARINE_SUBMITTERS} and related key.
+ * Submitter Manager is responsible for load {@link JobSubmitter} implements class.
*/
public class SubmitterManager {
private static final Logger LOG = LoggerFactory.getLogger(SubmitterManager.class);
- private SubmarineConfiguration conf;
- private ConcurrentMap<String, JobSubmitter> submitterMap = new ConcurrentHashMap<>();
+ private static final SubmitterManager INSTANCE = new SubmitterManager();
- public SubmitterManager(SubmarineConfiguration conf) {
- this.conf = conf;
- loadSubmitters();
+ private final Map<String, JobSubmitterConfig> SUBMITTER_TABLE = new HashMap<>();
+
+ private JobSubmitter submitter;
+
+ {
Review comment:
Using the initializing instance members to split the initialize logic and this approach can be used to share a block of code between multiple constructors. So that it can avoid some mistakes.
More info see https://docs.oracle.com/javase/tutorial/java/javaOO/initial.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services