[GitHub] [carbondata] marchpure opened a new pull request #3978: [CARBONDATA-4028] Fix failed to unlock during update

[GitBox]

marchpure opened a new pull request #3978:
URL: https://github.com/apache/carbondata/pull/3978


   
    ### Why is this PR needed?
   1. In the update flow, we unpresist dataset before unlocking. unlock will fail once the dataset unpresist is interrupted.
   2. cleanStaleDeltaFiles will hold the lock, which degrade the concurrency perf a lot.
   
    ### What changes were proposed in this PR?
   1. unlock before unpresisting dataset
   2. cleanStaleDeltaFiles won't hold the lock.
       
    ### 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 #3978:
URL: https://github.com/apache/carbondata/pull/3978#issuecomment-706906331


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


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


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


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



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/mutation/CarbonProjectForUpdateCommand.scala
##########
@@ -235,14 +237,38 @@ private[sql] case class CarbonProjectForUpdateCommand(
         }
         sys.error("Update operation failed. please check logs.")
     } finally {
-      if (null != dataSet && isPersistEnabled) {
-        dataSet.unpersist()
+      if (updateLock.unlock()) {
+        LOGGER.info(s"updateLock unlocked successfully after update operation $tableName")
+      } else {
+        LOGGER.error(s"Unable to unlock updateLock for table $tableName after table updation");

Review comment:
       ```suggestion
           LOGGER.error(s"Unable to unlock updateLock for table $tableName after table update");
   ```




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


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


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


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


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

marchpure commented on a change in pull request #3978:
URL: https://github.com/apache/carbondata/pull/3978#discussion_r503242850



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/mutation/CarbonProjectForUpdateCommand.scala
##########
@@ -235,14 +237,38 @@ private[sql] case class CarbonProjectForUpdateCommand(
         }
         sys.error("Update operation failed. please check logs.")
     } finally {
-      if (null != dataSet && isPersistEnabled) {
-        dataSet.unpersist()
+      if (updateLock.unlock()) {
+        LOGGER.info(s"updateLock unlocked successfully after update operation $tableName")
+      } else {
+        LOGGER.error(s"Unable to unlock updateLock for table $tableName after table updation");

Review comment:
       I have modified code according to your suggestion




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


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


   


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