You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2021/03/18 17:01:01 UTC

[GitHub] [incubator-dolphinscheduler] chengshiwen opened a new pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

chengshiwen opened a new pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104


   pr #5088
   issue #5087


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen edited a comment on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen edited a comment on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803258775


   > @chengshiwen @CalvinKirs
   > 
   > LIMIT limits the number of rows in a SQL query, and displayRows limits the number of rows that a log can print.
   > Isn't the number of queries the number of items to print?
   
   A user can choose not to send mail, so printing a few query results into the log is a good way. The variable `displayRows` is limited to less than 100, in order to avoid that the number of logs is too large.
   
   For sending mail, the number of rows of query results sent can be larger, for example `<= 10000`
   
   ![image](https://user-images.githubusercontent.com/4902714/111861295-08bb9200-8988-11eb-8efd-45938d7c5576.png)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r597561469



##########
File path: dolphinscheduler-alert/src/main/java/org/apache/dolphinscheduler/alert/utils/MailUtils.java
##########
@@ -85,15 +101,6 @@
      */
     public static Map<String,Object> sendMails(Collection<String> receivers, Collection<String> receiversCc, String title, String content, String showType) {
         Map<String,Object> retMap = new HashMap<>();
-
-        // if mail is default config, no need to process
-        if (StringUtils.isEmpty(MAIL_SERVER_HOST) || "xxx.xxx.com".equals(MAIL_SERVER_HOST)) {
-            retMap.put(Constants.MAIL_ENABLED, false);
-            retMap.put(Constants.STATUS, true);
-            return retMap;
-        }
-

Review comment:
       I think the legality of the parameters that should be sent here needs to be judged. Btw, tools should not include business, therefore, do not use judgments such as xxx.xxx.com.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] sonarcloud[bot] removed a comment on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-802139307


   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen edited a comment on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen edited a comment on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803258775


   > @chengshiwen @CalvinKirs
   > 
   > LIMIT limits the number of rows in a SQL query, and displayRows limits the number of rows that a log can print.
   > Isn't the number of queries the number of items to print?
   
   A user can choose not to send mail, so printing a few query results into the log is a good way. The variable `displayRows` is limited to less than 100, in order to avoid that the number of logs is too large.
   
   For sending mail, the number of rows of query results sent can be larger, for example `<= 10000`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803699297


   @zhuangchong You can start a discussion in the dev email, and realize this improvement according to the idea you mentioned, not on this 1.3.6-prepare. The current pr is a cherry-pick from dev, and this improvement is the result of the previous discussion on March 17.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803258775


   > @chengshiwen @CalvinKirs
   > 
   > LIMIT limits the number of rows in a SQL query, and displayRows limits the number of rows that a log can print.
   > Isn't the number of queries the number of items to print?
   
   A user can choose not to send mail, so printing a small amount of query results into the log is a good way. The variable `displayRows` is limited to less than 100, in order to avoid that the number of logs is too large.
   
   For sending mail, the number of rows of query results sent can be larger, for example `<= 10000`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] zhuangchong commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
zhuangchong commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803282384


   @chengshiwen @CalvinKirs 
   
   I wondered if this would make it more user-friendly,
   
   SQL query result display has the email and log printing, on the front page for users to choose more the result display, set up the article a SQL query number limit parameters, the user can select only the log printing way, also can only choose mail way, also can also choose one of two ways (choice is at the same time may produce a large amount of data, this can agree)
   In addition, I choose two display methods of log printing and email sending at the same time. I feel that the data can be viewed in either way many times at a time. Why should I view the data in both ways at the same time?
   
   ---
   
   我想了一下这样调整会不会对用户更友好一些，
   sql查询结果展示方式有邮件发送和日志打印，在前端页面可供用户进行多选结果展示方式，设置一个sql查询条数限制参数，用户可以只选择日志打印方式,也可以只选择邮件发送方式,也可以同时选择两种方式（同时选择的时候就是可能会产生很大的数据量，这个可以认同）
   
   并且同时选择日志打印和邮件发送两种展示方法，我感觉多次一举，在任何一种方式都可以查看数据，为什么要两种方式同时查看？
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r597610050



##########
File path: dolphinscheduler-alert/src/main/java/org/apache/dolphinscheduler/alert/utils/MailUtils.java
##########
@@ -85,15 +101,6 @@
      */
     public static Map<String,Object> sendMails(Collection<String> receivers, Collection<String> receiversCc, String title, String content, String showType) {
         Map<String,Object> retMap = new HashMap<>();
-
-        // if mail is default config, no need to process
-        if (StringUtils.isEmpty(MAIL_SERVER_HOST) || "xxx.xxx.com".equals(MAIL_SERVER_HOST)) {
-            retMap.put(Constants.MAIL_ENABLED, false);
-            retMap.put(Constants.STATUS, true);
-            return retMap;
-        }
-

Review comment:
       Because the 1.3.5 release version needed a quick fix, I made this modification, although I know it is unreasonable. Now I have removed it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r598066477



##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTask.java
##########
@@ -269,15 +291,20 @@ private void resultProcess(ResultSet resultSet) throws Exception{
             rowCount++;
         }
         String result = JSONUtils.toJsonString(resultJSONArray);
-        logger.debug("execute sql : {}", result);
+        logger.debug("execute sql result : {}", result);
+
+        int displayRows = sqlParameters.getDisplayRows() > 0 ? sqlParameters.getDisplayRows() : Constants.DEFAULT_DISPLAY_ROWS;
+        displayRows = Math.min(displayRows, resultJSONArray.size());
+        logger.info("display sql result {} rows as follows:", displayRows);
+        for (int i = 0; i < displayRows; i++) {
+            String row = JSONUtils.toJsonString(resultJSONArray.get(i));
+            logger.info("row {} : {}", i + 1, row);
+        }

Review comment:
       The variable `LIMIT` is different from `displayRows`, and the latter one is used to print in the log for users to view.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803247177


   @zhuangchong @break60 please take a look UI.thx


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] sonarcloud[bot] commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803261037


   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] xingchun-chen merged pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
xingchun-chen merged pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] zhuangchong commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
zhuangchong commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r598062616



##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/worker/task/sql/SqlTask.java
##########
@@ -269,15 +291,20 @@ private void resultProcess(ResultSet resultSet) throws Exception{
             rowCount++;
         }
         String result = JSONUtils.toJsonString(resultJSONArray);
-        logger.debug("execute sql : {}", result);
+        logger.debug("execute sql result : {}", result);
+
+        int displayRows = sqlParameters.getDisplayRows() > 0 ? sqlParameters.getDisplayRows() : Constants.DEFAULT_DISPLAY_ROWS;
+        displayRows = Math.min(displayRows, resultJSONArray.size());
+        logger.info("display sql result {} rows as follows:", displayRows);
+        for (int i = 0; i < displayRows; i++) {
+            String row = JSONUtils.toJsonString(resultJSONArray.get(i));
+            logger.info("row {} : {}", i + 1, row);
+        }

Review comment:
       Variable LIMIT and sqlParameters getDisplayRows () should be the same, please remove the variable LIMIT the relevant code




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] CalvinKirs commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r597452312



##########
File path: dolphinscheduler-alert/src/main/java/org/apache/dolphinscheduler/alert/utils/MailUtils.java
##########
@@ -85,15 +101,6 @@
      */
     public static Map<String,Object> sendMails(Collection<String> receivers, Collection<String> receiversCc, String title, String content, String showType) {
         Map<String,Object> retMap = new HashMap<>();
-
-        // if mail is default config, no need to process
-        if (StringUtils.isEmpty(MAIL_SERVER_HOST) || "xxx.xxx.com".equals(MAIL_SERVER_HOST)) {
-            retMap.put(Constants.MAIL_ENABLED, false);
-            retMap.put(Constants.STATUS, true);
-            return retMap;
-        }
-
-        retMap.put(Constants.MAIL_ENABLED, true);

Review comment:
       why delete this?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803258710


   > @chengshiwen @CalvinKirs
   > 
   > LIMIT limits the number of rows in a SQL query, and displayRows limits the number of rows that a log can print.
   > Isn't the number of queries the number of items to print?
   
   A user can choose not to send mail, so printing a small amount of query results into the log is a good way. The variable `displayRows` is limited to less than 100, in order to avoid that the number of logs is too large.
   
   For sending mail, the number of rows of query results sent can be larger, for example `<= 10000`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r597552978



##########
File path: dolphinscheduler-alert/src/main/java/org/apache/dolphinscheduler/alert/utils/MailUtils.java
##########
@@ -85,15 +101,6 @@
      */
     public static Map<String,Object> sendMails(Collection<String> receivers, Collection<String> receiversCc, String title, String content, String showType) {
         Map<String,Object> retMap = new HashMap<>();
-
-        // if mail is default config, no need to process
-        if (StringUtils.isEmpty(MAIL_SERVER_HOST) || "xxx.xxx.com".equals(MAIL_SERVER_HOST)) {
-            retMap.put(Constants.MAIL_ENABLED, false);
-            retMap.put(Constants.STATUS, true);
-            return retMap;
-        }
-
-        retMap.put(Constants.MAIL_ENABLED, true);

Review comment:
       These lines were added in last `1.3.5` version, in order to solve the problem of `sending mail failed`, now they are not needed




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r597610050



##########
File path: dolphinscheduler-alert/src/main/java/org/apache/dolphinscheduler/alert/utils/MailUtils.java
##########
@@ -85,15 +101,6 @@
      */
     public static Map<String,Object> sendMails(Collection<String> receivers, Collection<String> receiversCc, String title, String content, String showType) {
         Map<String,Object> retMap = new HashMap<>();
-
-        // if mail is default config, no need to process
-        if (StringUtils.isEmpty(MAIL_SERVER_HOST) || "xxx.xxx.com".equals(MAIL_SERVER_HOST)) {
-            retMap.put(Constants.MAIL_ENABLED, false);
-            retMap.put(Constants.STATUS, true);
-            return retMap;
-        }
-

Review comment:
       Because the 1.3.5 release version needed a quick fix, I made this modification, although I know it is unreasonable. Now I have removed it. And in the current code, there is no need to judge the parameters




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] sonarcloud[bot] commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-802139307


   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5104&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5104&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen commented on a change in pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen commented on a change in pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#discussion_r597552978



##########
File path: dolphinscheduler-alert/src/main/java/org/apache/dolphinscheduler/alert/utils/MailUtils.java
##########
@@ -85,15 +101,6 @@
      */
     public static Map<String,Object> sendMails(Collection<String> receivers, Collection<String> receiversCc, String title, String content, String showType) {
         Map<String,Object> retMap = new HashMap<>();
-
-        // if mail is default config, no need to process
-        if (StringUtils.isEmpty(MAIL_SERVER_HOST) || "xxx.xxx.com".equals(MAIL_SERVER_HOST)) {
-            retMap.put(Constants.MAIL_ENABLED, false);
-            retMap.put(Constants.STATUS, true);
-            return retMap;
-        }
-
-        retMap.put(Constants.MAIL_ENABLED, true);

Review comment:
       These lines were added by me in last `1.3.5` version, in order to solve the problem of `sending mail failed`, now they are not needed




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] zhuangchong commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
zhuangchong commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803257175


   @chengshiwen @CalvinKirs 
   
   LIMIT limits the number of rows in a SQL query, and displayRows limits the number of rows that a log can print.
   Isn't the number of queries the number of items to print?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] chengshiwen removed a comment on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
chengshiwen removed a comment on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803258710


   > @chengshiwen @CalvinKirs
   > 
   > LIMIT limits the number of rows in a SQL query, and displayRows limits the number of rows that a log can print.
   > Isn't the number of queries the number of items to print?
   
   A user can choose not to send mail, so printing a small amount of query results into the log is a good way. The variable `displayRows` is limited to less than 100, in order to avoid that the number of logs is too large.
   
   For sending mail, the number of rows of query results sent can be larger, for example `<= 10000`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-dolphinscheduler] zhuangchong commented on pull request #5104: [1.3.6-prepare][Feature-5087][SqlTask] Add a switch to send mail and print head logs in SqlTask #5088

Posted by GitBox <gi...@apache.org>.
zhuangchong commented on pull request #5104:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/5104#issuecomment-803704453


   > @zhuangchong You can start a discussion in the dev email, and realize this improvement according to the idea you mentioned, not on this 1.3.6-prepare. The current pr is a cherry-pick from dev, and this improvement is the result of the previous discussion on March 17.
   
   ok, no problem.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org