[GitHub] [carbondata] QiangCai opened a new pull request #3825: [CARBONDATA-3889] Optimize code in carbondata-common module

[GitBox]

QiangCai opened a new pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825


    ### Why is this PR needed?
   1. Field can be 'final'
   2. Redundant 'if' statement
   3. Empty method
   4. Redundant 'throws' clause
   5. Anonymous type can be replaced with lambda
   6. Typo
   7. Redundant String operation
   
    ### What changes were proposed in this PR?
   1. change field to be 'final'
   2. change redundant 'if' statement
   3. remove Empty method
   4. remove redundant 'throws' clause
   5. change anonymous type  to lambda
   6. fix typo
   7. remove redundant String operation
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - No
   
       
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-653777264


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1565/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-653777356


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3302/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

QiangCai commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-653824326


   retest this please


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-653832863


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1567/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-653833148


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3304/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

kevinjmh commented on a change in pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#discussion_r451468217



##########
File path: common/src/main/java/org/apache/carbondata/common/logging/impl/ExtendedRollingFileAppender.java
##########
@@ -203,19 +194,16 @@ public void rollOver() {
   private void cleanUpLogs(final String startName, final String folderPath) {
     if (maxBackupIndex > 0) {
       // Clean the logs files
-      Runnable r = new Runnable() {
-
-        public void run() {
-          synchronized (ExtendedRollingFileAppender.class) {
-            cleanupInProgress = true;
-            try {
-              cleanLogs(startName, folderPath, maxBackupIndex);
-            } catch (Throwable e) {
-              // ignore any error
-              LogLog.error("Cleaning logs failed", e);
-            } finally {
-              cleanupInProgress = false;
-            }
+      Runnable r = () -> {

Review comment:
       original one maybe more friendly to read




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

kevinjmh commented on a change in pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#discussion_r451468217



##########
File path: common/src/main/java/org/apache/carbondata/common/logging/impl/ExtendedRollingFileAppender.java
##########
@@ -203,19 +194,16 @@ public void rollOver() {
   private void cleanUpLogs(final String startName, final String folderPath) {
     if (maxBackupIndex > 0) {
       // Clean the logs files
-      Runnable r = new Runnable() {
-
-        public void run() {
-          synchronized (ExtendedRollingFileAppender.class) {
-            cleanupInProgress = true;
-            try {
-              cleanLogs(startName, folderPath, maxBackupIndex);
-            } catch (Throwable e) {
-              // ignore any error
-              LogLog.error("Cleaning logs failed", e);
-            } finally {
-              cleanupInProgress = false;
-            }
+      Runnable r = () -> {

Review comment:
       original one maybe more friendly to read. Unless we are targeting on JDK8 from now on :)




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

QiangCai commented on a change in pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#discussion_r456247931



##########
File path: common/src/main/java/org/apache/carbondata/common/logging/impl/ExtendedRollingFileAppender.java
##########
@@ -203,19 +194,16 @@ public void rollOver() {
   private void cleanUpLogs(final String startName, final String folderPath) {
     if (maxBackupIndex > 0) {
       // Clean the logs files
-      Runnable r = new Runnable() {
-
-        public void run() {
-          synchronized (ExtendedRollingFileAppender.class) {
-            cleanupInProgress = true;
-            try {
-              cleanLogs(startName, folderPath, maxBackupIndex);
-            } catch (Throwable e) {
-              // ignore any error
-              LogLog.error("Cleaning logs failed", e);
-            } finally {
-              cleanupInProgress = false;
-            }
+      Runnable r = () -> {

Review comment:
       Reverted




----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-659977333


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/1671/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

CarbonDataQA1 commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-659978662


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/3413/
   


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

kevinjmh commented on pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825#issuecomment-659983455


   LGTM


----------------------------------------------------------------
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:
[hidden email]

[GitBox]

asfgit closed pull request #3825:
URL: https://github.com/apache/carbondata/pull/3825


   


----------------------------------------------------------------
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:
[hidden email]