Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-1261. Put configurations into Kubernetes Configmap #969

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

raykuo18
Copy link
Contributor

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
Copy link
Contributor Author

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

@cdmikechen
Copy link
Contributor

cdmikechen commented Jun 20, 2022

    // 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
Copy link
Contributor

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
Copy link
Contributor

cdmikechen commented Jun 20, 2022

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants