Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Aug 7, 2020
Merged

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

merged 3 commits into from
Aug 7, 2020

Conversation

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

LGTM.

@zhuangchong
Copy link
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
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
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
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
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~

@sonarcloud
Copy link

sonarcloud bot commented Jul 29, 2020

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
Member

@gabrywu gabrywu left a comment

Choose a reason for hiding this comment

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

+1

@gabrywu gabrywu 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