[GitHub] [carbondata] maheshrajus opened a new pull request #3808: [CARBONDATA-3873] Secondary index compaction with maintable clean files causing exception

[GitBox]

maheshrajus opened a new pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808


   [CARBONDATA-3873] Secondary index compaction with maintable clean files causing exception
    ### Why is this PR needed?
    Secondary index compaction with maintable clean files causing exception
   
    ### What changes were proposed in this PR?
   
   Secondary index compaction with main table clean files causing an exception
   If any compaction is failed and clean files causing the exception.
   If any compaction is failed then disable all SI tables which are successful.
    ### 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 #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-650178820


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


----------------------------------------------------------------
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 #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-650179402


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


----------------------------------------------------------------
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 #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-652278446


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


----------------------------------------------------------------
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 #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-652279732


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


----------------------------------------------------------------
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]

akashrn5 commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r448761040



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/load/Compactor.scala
##########
@@ -121,10 +124,17 @@ object Compactor {
           segmentIdToLoadStartTimeMapping(validSegments.head),
           SegmentStatus.SUCCESS,
           carbonLoadModelForMergeDataFiles.getFactTimeStamp, rebuiltSegments.toList.asJava)
-
+        siCompactionIndexList ::= indexCarbonTable
       } catch {
         case ex: Exception =>
           LOGGER.error(s"Compaction failed for SI table ${secondaryIndex.indexName}", ex)
+          siCompactionIndexList.foreach { indexCarbonTable =>
+            sparkSession.sql(
+              s"""ALTER TABLE ${carbonLoadModel.getDatabaseName}.${

Review comment:
       please correct the format for sparksession.sql(""), please check the other files where we do alter set for SI, can follow the same

##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java
##########
@@ -455,7 +455,7 @@ public boolean createNewLockFile() throws IOException {
     try {
       listStatus = fileSystem.listStatus(path);
     } catch (IOException e) {
-      LOGGER.error("Exception occured: " + e.getMessage(), e);

Review comment:
       revert this change if not rerquired

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/load/Compactor.scala
##########
@@ -121,10 +124,17 @@ object Compactor {
           segmentIdToLoadStartTimeMapping(validSegments.head),
           SegmentStatus.SUCCESS,
           carbonLoadModelForMergeDataFiles.getFactTimeStamp, rebuiltSegments.toList.asJava)
-
+        siCompactionIndexList ::= indexCarbonTable
       } catch {
         case ex: Exception =>
           LOGGER.error(s"Compaction failed for SI table ${secondaryIndex.indexName}", ex)
+          siCompactionIndexList.foreach { indexCarbonTable =>

Review comment:
       please add a comment here, what you are doing in which scenario




----------------------------------------------------------------
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]

VenuReddy2103 commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r449989095



##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java
##########
@@ -455,7 +455,7 @@ public boolean createNewLockFile() throws IOException {
     try {
       listStatus = fileSystem.listStatus(path);
     } catch (IOException e) {
-      LOGGER.error("Exception occured: " + e.getMessage(), e);

Review comment:
       @akashrn5  Every time we create a new table/index/mv, we get this error log in the list files. This is the normal flow. Instead of error, warn is better to avoid confusion. I think, this looks ok. May be @maheshrajus can show this change details in the PR description.




----------------------------------------------------------------
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]

maheshrajus commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r450299932



##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/filesystem/AbstractDFSCarbonFile.java
##########
@@ -455,7 +455,7 @@ public boolean createNewLockFile() throws IOException {
     try {
       listStatus = fileSystem.listStatus(path);
     } catch (IOException e) {
-      LOGGER.error("Exception occured: " + e.getMessage(), e);

Review comment:
       
   @akashrn5 @VenuReddy2103  i will add details in PR description




----------------------------------------------------------------
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]

maheshrajus commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r450300989



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/load/Compactor.scala
##########
@@ -121,10 +124,17 @@ object Compactor {
           segmentIdToLoadStartTimeMapping(validSegments.head),
           SegmentStatus.SUCCESS,
           carbonLoadModelForMergeDataFiles.getFactTimeStamp, rebuiltSegments.toList.asJava)
-
+        siCompactionIndexList ::= indexCarbonTable
       } catch {
         case ex: Exception =>
           LOGGER.error(s"Compaction failed for SI table ${secondaryIndex.indexName}", ex)
+          siCompactionIndexList.foreach { indexCarbonTable =>

Review comment:
       OK




----------------------------------------------------------------
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]

maheshrajus commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r450301066



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/load/Compactor.scala
##########
@@ -121,10 +124,17 @@ object Compactor {
           segmentIdToLoadStartTimeMapping(validSegments.head),
           SegmentStatus.SUCCESS,
           carbonLoadModelForMergeDataFiles.getFactTimeStamp, rebuiltSegments.toList.asJava)
-
+        siCompactionIndexList ::= indexCarbonTable
       } catch {
         case ex: Exception =>
           LOGGER.error(s"Compaction failed for SI table ${secondaryIndex.indexName}", ex)
+          siCompactionIndexList.foreach { indexCarbonTable =>
+            sparkSession.sql(
+              s"""ALTER TABLE ${carbonLoadModel.getDatabaseName}.${

Review comment:
       OK




----------------------------------------------------------------
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]

akashrn5 commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r450360202



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/load/Compactor.scala
##########
@@ -121,10 +124,20 @@ object Compactor {
           segmentIdToLoadStartTimeMapping(validSegments.head),
           SegmentStatus.SUCCESS,
           carbonLoadModelForMergeDataFiles.getFactTimeStamp, rebuiltSegments.toList.asJava)
-
+        siCompactionIndexList ::= indexCarbonTable
       } catch {
         case ex: Exception =>
           LOGGER.error(s"Compaction failed for SI table ${secondaryIndex.indexName}", ex)
+          // If any compaction is failed then make all SI disabled which are success.
+          // They will be enabled in next load
+          siCompactionIndexList.foreach { indexCarbonTable =>
+            sparkSession.sql(
+              s"""ALTER TABLE ${

Review comment:
       format is not right, please correct 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:
[hidden email]

[GitBox]

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


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


----------------------------------------------------------------
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 #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-654390139


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


----------------------------------------------------------------
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]

akashrn5 commented on a change in pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#discussion_r450658081



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/load/Compactor.scala
##########
@@ -121,10 +124,20 @@ object Compactor {
           segmentIdToLoadStartTimeMapping(validSegments.head),
           SegmentStatus.SUCCESS,
           carbonLoadModelForMergeDataFiles.getFactTimeStamp, rebuiltSegments.toList.asJava)
-
+        siCompactionIndexList ::= indexCarbonTable
       } catch {
         case ex: Exception =>
           LOGGER.error(s"Compaction failed for SI table ${secondaryIndex.indexName}", ex)
+          // If any compaction is failed then make all SI disabled which are success.
+          // They will be enabled in next load
+          siCompactionIndexList.foreach { indexCarbonTable =>
+            sparkSession.sql(
+              s"""
+                 | ALTER TABLE ${carbonLoadModel.getDatabaseName}.${indexCarbonTable.getTableName}
+                 | SET
+                 | SERDEPROPERTIES ('isSITableEnabled' = 'false')

Review comment:
       move this line above




----------------------------------------------------------------
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]

akashrn5 commented on pull request #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-654663162


   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]

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


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


----------------------------------------------------------------
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 #3808:
URL: https://github.com/apache/carbondata/pull/3808#issuecomment-654734465


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


----------------------------------------------------------------
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 #3808:
URL: https://github.com/apache/carbondata/pull/3808


   


----------------------------------------------------------------
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]