Skip to content

[Fix] [Server] Fix the spell error and add log in SqlTask#3243

Merged
gabry-lab merged 3 commits intoapache:devfrom
zixi0825:fix_spell_error_and_add_log
Aug 7, 2020
Merged

[Fix] [Server] Fix the spell error and add log in SqlTask#3243
gabry-lab merged 3 commits intoapache:devfrom
zixi0825:fix_spell_error_and_add_log

Conversation

@zixi0825
Copy link
Copy Markdown
Member

What is the purpose of the pull request

This pull request fix the spell error 、add log in SqlTask and add null judgment in SqlTask

Brief change log

  • none

Verify this pull request

none

Copy link
Copy Markdown
Member

@CalvinKirs CalvinKirs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@yangyichao-mango
Copy link
Copy Markdown
Contributor

LGTM.

@zhuangchong
Copy link
Copy Markdown
Contributor

  1. SqlParameters = = null, because the key parameters have been checked in the UI, where SqlParameters cannot be null

  2. For the exception of connection closing, the log should be added. In my opinion, if an exception occurs, the connection = = null


  1. sqlParameters ==null , 因为在UI里面已经对关键参数进行校验,此处sqlParameters不可能为null
    2.针对connection关闭发生异常增加日志,我认为更应该如果发生异常将connection==null

@yangyichao-mango
Copy link
Copy Markdown
Contributor

  1. SqlParameters = = null, because the key parameters have been checked in the UI, where SqlParameters cannot be null

  2. For the exception of connection closing, the log should be added. In my opinion, if an exception occurs, the connection = = null

  3. sqlParameters ==null , 因为在UI里面已经对关键参数进行校验,此处sqlParameters不可能为null
    2.针对connection关闭发生异常增加日志,我认为更应该如果发生异常将connection==null

Hi,

  1. In addition to using webui, users can also use API to create directly, and it can also solve the null value caused by errors in the JSON serialization process. Judging null value here actually can enhance the robustness of the code.
    Example: Although the value of a String cannot be null, we usually use the StringUtils.isNotEmpty, and it add the null value judgment.
  2. Please point out where the code is.

1.用户除了使用webui,也可以使用api直接创建,除此之外可以将将json序列化过程中出错导致的null值给解决。这里判断是否为null,可以增强代码鲁棒性。举例:虽然一个string的值不可能为null,我们通常使用的StringUtils.isNotEmpty中依然加了null值判断。
2.可以指出具体是哪段代码嘛。

If you have any question or suggestion, welcome to put forward~
Thx a lot~

@zhuangchong
Copy link
Copy Markdown
Contributor

zhuangchong commented Jul 22, 2020

@yangyichao-mango

  1. I haven't used the method of creating API directly. It's not clear. I see that the method of checkprocessnodelist in dolphinscheduler-api module can also detect parameters. I don't know if this method will be used to create API directly. You can have a look.
public static boolean checkTaskNodeParameters(String parameter, String taskType) {
    AbstractParameters abstractParameters = TaskParametersUtils.getParameters(taskType, parameter);

    if (abstractParameters != null) {
      return abstractParameters.checkParameters();
    }

    return false;
  }

@yangyichao-mango

1. I haven't used the method of creating API directly. It's not clear. I see that the method of checkprocessnodelist in dolphinscheduler-api  module can also detect parameters. I don't know if this method will be used to create API directly. You can have a look.   


  1. 你说的api直接创建这种方式我没有用过,这个不太清楚,我看dolphinscheduler-api 模块里面有checkProcessNodeList方法也会检测参数。不知道你说的api直接创建会不会走这个方法,你可以看一下。
public static boolean checkTaskNodeParameters(String parameter, String taskType) {
    AbstractParameters abstractParameters = TaskParametersUtils.getParameters(taskType, parameter);

    if (abstractParameters != null) {
      return abstractParameters.checkParameters();
    }

    return false;
  }

@zhuangchong
Copy link
Copy Markdown
Contributor

If SqlParameters is judged to be null, are all abstractparameters required? If so, they need to be abstracted

如果SqlParameters判断为空,那么是不是所有的 AbstractParameters的都需要,如果是,就需要抽象出来

@yangyichao-mango
Copy link
Copy Markdown
Contributor

yangyichao-mango commented Jul 22, 2020

If SqlParameters is judged to be null, are all abstractparameters required? If so, they need to be abstracted

如果SqlParameters判断为空,那么是不是所有的 AbstractParameters的都需要,如果是,就需要抽象出来

I agree with you, if you are interested in this, I think it is better to create an issue and describe all the situation, this pr is just for spell error and some log info. Thx a lot~

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Copy Markdown
Member

@gabry-lab gabry-lab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@gabry-lab gabry-lab merged commit 40a2181 into apache:dev Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants