[GitHub] [carbondata] Pickupolddriver opened a new pull request #3935: [CARBONDATA-3993] Remove deletePartialLoadData in data loading&compaction process

[GitBox]

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


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


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


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


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


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



##########
File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java
##########
@@ -227,34 +224,9 @@ public static boolean deleteLoadFoldersFromFileSystem(
     if (details != null && details.length != 0) {
       for (LoadMetadataDetails oneLoad : details) {
         if (checkIfLoadCanBeDeleted(oneLoad, isForceDelete)) {
-          ICarbonLock segmentLock = CarbonLockFactory.getCarbonLockObj(absoluteTableIdentifier,

Review comment:
       after this PR, method deleteLoadFoldersFromFileSystem will be invoked by clean files only, right?
   if yes, why we remove this 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:
[hidden email]

[GitBox]

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



##########
File path: processing/src/main/java/org/apache/carbondata/processing/util/CarbonLoaderUtil.java
##########
@@ -323,10 +320,6 @@ public static boolean recordNewLoadMetadata(LoadMetadataDetails newMetaEntry,
             for (LoadMetadataDetails entry : listOfLoadFolderDetails) {

Review comment:
       Let's discuss insertOverwrite behavior.
   Should we delete old data immediately?
   




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

Pickupolddriver commented on a change in pull request #3935:
URL: https://github.com/apache/carbondata/pull/3935#discussion_r511703571



##########
File path: processing/src/main/java/org/apache/carbondata/processing/util/CarbonLoaderUtil.java
##########
@@ -323,10 +320,6 @@ public static boolean recordNewLoadMetadata(LoadMetadataDetails newMetaEntry,
             for (LoadMetadataDetails entry : listOfLoadFolderDetails) {

Review comment:
       Seems in `insertOverwrite` process, the stale data can be removed immediately.  I think we can keep the original `addToStaleFolders` function here
   
   > Let's discuss insertOverwrite behavior.
   > Should we delete old data immediately?
   
   Seems in `insertOverwrite` process, the stale data can be removed immediately.  I think we can keep the original `addToStaleFolders` function here




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

Pickupolddriver commented on a change in pull request #3935:
URL: https://github.com/apache/carbondata/pull/3935#discussion_r511703719



##########
File path: processing/src/main/java/org/apache/carbondata/processing/util/CarbonLoaderUtil.java
##########
@@ -323,10 +320,6 @@ public static boolean recordNewLoadMetadata(LoadMetadataDetails newMetaEntry,
             for (LoadMetadataDetails entry : listOfLoadFolderDetails) {

Review comment:
       > Let's discuss insertOverwrite behavior.
   > Should we delete old data immediately?
   
   Seems in `insertOverwrite` process, the stale data can be removed immediately.  I think we can keep the original `addToStaleFolders` function here




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

Pickupolddriver commented on a change in pull request #3935:
URL: https://github.com/apache/carbondata/pull/3935#discussion_r511703767



##########
File path: processing/src/main/java/org/apache/carbondata/processing/util/CarbonLoaderUtil.java
##########
@@ -323,10 +320,6 @@ public static boolean recordNewLoadMetadata(LoadMetadataDetails newMetaEntry,
             for (LoadMetadataDetails entry : listOfLoadFolderDetails) {

Review comment:
       Seems in `insertOverwrite` process, the stale data can be removed immediately.  I think we can keep the original `addToStaleFolders` function here




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

Pickupolddriver commented on a change in pull request #3935:
URL: https://github.com/apache/carbondata/pull/3935#discussion_r511705764



##########
File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java
##########
@@ -227,34 +224,9 @@ public static boolean deleteLoadFoldersFromFileSystem(
     if (details != null && details.length != 0) {
       for (LoadMetadataDetails oneLoad : details) {
         if (checkIfLoadCanBeDeleted(oneLoad, isForceDelete)) {
-          ICarbonLock segmentLock = CarbonLockFactory.getCarbonLockObj(absoluteTableIdentifier,

Review comment:
       Actually, this part of the modifications comes from #3971 contributed by @Indhumathi27 .  It removes the condition of insert_in_progress and lock acquirement, I think it may delete the segments which are in progress. So maybe I can revoke this modification.




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


   @Pickupolddriver I will rework by basing on this PR and raise another PR.  Please close this PR, thanks for your contribution.


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

Pickupolddriver commented on pull request #3935:
URL: https://github.com/apache/carbondata/pull/3935#issuecomment-726555194


   > @Pickupolddriver I will rework by basing on this PR and raise another PR. Please close this PR, thanks for your contribution.
   
   OK, PR closed.


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

Pickupolddriver closed pull request #3935:
URL: https://github.com/apache/carbondata/pull/3935


   


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