submarine icon indicating copy to clipboard operation
submarine copied to clipboard

SUBMARINE-1261. Put configurations into Kubernetes Configmap

Open raykuo18 opened this issue 3 years ago • 4 comments

What is this PR for?

Collect configurations in conf/submarine-sit.xml, common-utils/SubmarineConfiguration.java, and common-utils/SubmarineConfVars into Kubernetes Configmap.

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-1261?filter=allissues

Questions:

  • Do the license files need updating? No
  • Are there breaking changes for older versions? No
  • Does this need new documentation? No

raykuo18 avatar Jun 19 '22 15:06 raykuo18

@KUAN-HSUN-LI Please help me review the code

raykuo18 avatar Jun 19 '22 16:06 raykuo18

    // Debug
    LOG.info("////////////////////    setupJettyServer Debug    //////////////////");
    LOG.info("SUBMARINE_SERVER_JETTY_THREAD_POOL_MAX: " +
        Integer.toString(conf.getInt(SubmarineConfVars.ConfVars.SUBMARINE_SERVER_JETTY_THREAD_POOL_MAX)));

If we explicitly use debug log in this part, I suggest changing the log level to debug/trace, for example:

if (LOG.isDebugEnabled()) {
    LOG.debug("xxxxx");
}

cdmikechen avatar Jun 20 '22 00:06 cdmikechen

jdbc.url: "jdbc:mysql://127.0.0.1:3306/submarine?useUnicode=true&characterEncoding=UTF-8&autoReconnect=true&allowMultiQueries=true&failOverReadOnly=false&zeroDateTimeBehavior=convertToNull&useSSL=false&serverTimezone=UTC&useTimezone=true&useLegacyDatetimeCode=true"

If we choose to replace XML configuration based on ConfigMap, I suggest changing 127.0.0.1 to MySQL service name. And it is better to provide a method/solution so that developers can easily develop and debug on their computers.

cdmikechen avatar Jun 20 '22 00:06 cdmikechen

ConfVars(String varName, VarType type) {
  switch(type) {
    case STRING:
      this.varName = varName;
      this.varClass = String.class;
      if (varName == "submarine.server.ssl.key.manager.password" ||
          varName == "submarine.server.ssl.truststore.path" || 
          varName == "submarine.server.ssl.truststore.type" ||
          varName == "submarine.server.ssl.truststore.password"
      ) {
        this.stringValue = null;
      } else {
        this.stringValue = System.getenv(varName);
      }
      this.intValue = -1;
      this.floatValue = -1;
      this.longValue = -1;
      this.booleanValue = false;
      break;

I don't think it is a good way to remove the default value from the method parameters and use if/else to judge. Do we have a better solution to replace this logic?

cdmikechen avatar Jun 20 '22 00:06 cdmikechen