[GitHub] carbondata pull request #2560: [HOTFIX] Removed file existence check to impr...

[qiuchenjian-2]

GitHub user manishgupta88 opened a pull request:

    https://github.com/apache/carbondata/pull/2560

    [HOTFIX] Removed file existence check to improve dataMap loading performance

    **Problem**
    DataMap loading performance degraded after adding file existence check.
   
    **Analysis**
    When carbonIndex file is read and  carbondata file path to its metadata Info map is prepared, file physical existence is getting checked every time which in case of HDFS file system is a namenode call. This degrades the dataMap loading performance. This is done to avoid failures for 2 scenarios
    1. Compatibility with 1.3 version store where segment file contains mergeIndex as well as index file name even though index file physically do not exist after creation of merge Index file.
    2. Handle IUD scenario where after delete operation carbondata file is deleted but the entry still exists in index file.
   
    **Fix**
    Modified code to check for physical file existence only in case when any IUD operation has happened on the table
   
     - [ ] Any interfaces changed?
     No
     - [ ] Any backward compatibility impacted?
     No
     - [ ] Document update required?
    No
     - [ ] Testing done
    Verified in cluster on 20 billion data      
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
    No


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/manishgupta88/carbondata query_slow_executor_pruning

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2560.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2560
   
----
commit 7081c8d480f580414583beef8798a5e3a208f549
Author: manishgupta88 <tomanishgupta18@...>
Date:   2018-07-25T14:18:41Z

    Removed file existence check to improve dataMap loading performance

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7510/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6262/



---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7520/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6275/



---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2560#discussion_r205411295
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/BlockletDataMapIndexStore.java ---
    @@ -81,6 +82,8 @@ public BlockletDataMapIndexWrapper get(TableBlockIndexUniqueIdentifierWrapper id
         List<BlockDataMap> dataMaps = new ArrayList<>();
         if (blockletDataMapIndexWrapper == null) {
           try {
    +        boolean isCarbonTableUpdated =
    --- End diff --
   
    Why not list files inside segment path once to list al carbodata files and check the existance in that list?


---

[qiuchenjian-2]

Github user manishgupta88 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2560#discussion_r205436561
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/BlockletDataMapIndexStore.java ---
    @@ -81,6 +82,8 @@ public BlockletDataMapIndexWrapper get(TableBlockIndexUniqueIdentifierWrapper id
         List<BlockDataMap> dataMaps = new ArrayList<>();
         if (blockletDataMapIndexWrapper == null) {
           try {
    +        boolean isCarbonTableUpdated =
    --- End diff --
   
    Listing files from metadata folder will be less costly as compared to listing files from segment path. Metadata contains only few files but segment path can contain lot of files which will do more RPC calls to namenode
    Also listing from segment file path will be very costly in case of flat folder structure OR when storage file system is S3


---

[qiuchenjian-2]

Github user ravipesala commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2560#discussion_r205666480
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/BlockletDataMapIndexStore.java ---
    @@ -81,6 +82,8 @@ public BlockletDataMapIndexWrapper get(TableBlockIndexUniqueIdentifierWrapper id
         List<BlockDataMap> dataMaps = new ArrayList<>();
         if (blockletDataMapIndexWrapper == null) {
           try {
    +        boolean isCarbonTableUpdated =
    --- End diff --
   
    Already list files done to get the locations of files, so better check in the map `carbonDataFileBlockMetaInfoMapping`


---

[qiuchenjian-2]

Github user manishgupta88 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2560#discussion_r205677203
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/BlockletDataMapIndexStore.java ---
    @@ -81,6 +82,8 @@ public BlockletDataMapIndexWrapper get(TableBlockIndexUniqueIdentifierWrapper id
         List<BlockDataMap> dataMaps = new ArrayList<>();
         if (blockletDataMapIndexWrapper == null) {
           try {
    +        boolean isCarbonTableUpdated =
    --- End diff --
   
    yes I have checked...I will modify the code and test the performance and then make the changes in PR


---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    @ravipesala ...handled review comments...kindly review and merge


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7562/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    LGTM


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6320/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/7571/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6324/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/6328/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6032/



---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2560
 
    retest this please


---