[GitHub] [carbondata] kunal642 opened a new pull request #3610: [HOTFIX] Concurrent insert test case failure fix

[GitBox]

kunal642 opened a new pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610
 
 
    ### Why is this PR needed?
   System.currentTimeMillis() is giving same results for 2 insert due to which 1 load is deleting the temp folder for another load.
   
    ### What changes were proposed in this PR?
   1. Change to System.nanoTime()
   2. If schema file does not exists then go for lookup which can induce some difference in the concurrent insert operations
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584500004
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/228/
   

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#discussion_r377471467
 
 

 ##########
 File path: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala
 ##########
 @@ -290,7 +290,7 @@ case class CarbonLoadDataCommand(
           FileFactory.mkdirs(metadataDirectoryPath)
         }
       } else {
-        carbonLoadModel.setSegmentId(System.currentTimeMillis().toString)
+        carbonLoadModel.setSegmentId(System.nanoTime().toString)
 
 Review comment:
   Many times have observed (mainly from user environment) that , two concurrent tasks can get same nano time. So, we use UUID in these scenario. But for segmentID we cannot use as it as may be  segment id will be casted to long.
   
   Need other solutions or go back to old look up logic

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#discussion_r377471467
 
 

 ##########
 File path: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala
 ##########
 @@ -290,7 +290,7 @@ case class CarbonLoadDataCommand(
           FileFactory.mkdirs(metadataDirectoryPath)
         }
       } else {
-        carbonLoadModel.setSegmentId(System.currentTimeMillis().toString)
+        carbonLoadModel.setSegmentId(System.nanoTime().toString)
 
 Review comment:
   Many times have observed (mainly from user environment) that , two concurrent tasks can get same nano time. So, we use UUID in these scenario. But for segmentID we cannot use as it as may be  segment id will be casted to long.
   
   Need other solutions or go back to old look up logic before #3601

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#discussion_r377471467
 
 

 ##########
 File path: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala
 ##########
 @@ -290,7 +290,7 @@ case class CarbonLoadDataCommand(
           FileFactory.mkdirs(metadataDirectoryPath)
         }
       } else {
-        carbonLoadModel.setSegmentId(System.currentTimeMillis().toString)
+        carbonLoadModel.setSegmentId(System.nanoTime().toString)
 
 Review comment:
   Many times have observed (mainly from user environment) that , two concurrent tasks can get same nano time. So, we use UUID in these scenario. But for segmentID we cannot use as it may be  casted to long.
   
   Need other solutions or go back to old look up logic before #3601

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on a change in pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#discussion_r377471467
 
 

 ##########
 File path: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala
 ##########
 @@ -290,7 +290,7 @@ case class CarbonLoadDataCommand(
           FileFactory.mkdirs(metadataDirectoryPath)
         }
       } else {
-        carbonLoadModel.setSegmentId(System.currentTimeMillis().toString)
+        carbonLoadModel.setSegmentId(System.nanoTime().toString)
 
 Review comment:
   Many times have observed (mainly from user environment) that , two concurrent tasks can get same nano time. So, we use UUID in these scenario. But for segmentID we cannot use as it may be  casted to long.
   
   Need other solutions or go back to old look up logic before #3601 for non-transactional tables

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584514631
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1930/
   

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


With regards,
Apache Git Services

[GitBox]

kunal642 commented on a change in pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#discussion_r377494474
 
 

 ##########
 File path: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonLoadDataCommand.scala
 ##########
 @@ -290,7 +290,7 @@ case class CarbonLoadDataCommand(
           FileFactory.mkdirs(metadataDirectoryPath)
         }
       } else {
-        carbonLoadModel.setSegmentId(System.currentTimeMillis().toString)
+        carbonLoadModel.setSegmentId(System.nanoTime().toString)
 
 Review comment:
   Added lookup for non-transactional also..

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584530366
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/231/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584551923
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1933/
   

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


With regards,
Apache Git Services

[GitBox]

kunal642 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584552155
 
 
   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]


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584561496
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/233/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584584547
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/1935/
   

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


With regards,
Apache Git Services

[GitBox]

ajantha-bhat commented on issue #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610#issuecomment-584596179
 
 
   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]


With regards,
Apache Git Services

[GitBox]

asfgit closed pull request #3610: [HOTFIX] Concurrent insert test case failure fix
URL: https://github.com/apache/carbondata/pull/3610
 
 
   

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


With regards,
Apache Git Services