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

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