[GitHub] carbondata pull request #2244: [CARBONDATA-2310] Refactored code to improve ...

[qiuchenjian-2]

GitHub user dhatchayani opened a pull request:

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

    [CARBONDATA-2310] Refactored code to improve Distributable interface

    [CARBONDATA-2362] Changing the Cacheable object from DataMap to Wrapper
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


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

    $ git pull https://github.com/dhatchayani/carbondata CARBONDATA-2362

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

    https://github.com/apache/carbondata/pull/2244.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 #2244
   
----
commit a2fa54a69c2ccd4edf4016e28cef7b5d2890aa60
Author: dhatchayani <dhatcha.official@...>
Date:   2018-04-27T17:33:52Z

    [CARBONDATA-2310] Refactored code to improve Distributable interface
    [CARBONDATA-2362] Changing the Cacheable object from DataMap to Wrapper

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

    https://github.com/apache/carbondata/pull/2244
 
    Retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2244
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4603/



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

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


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2244
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/4605/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2244#discussion_r184911430
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/dev/expr/DataMapExprWrapperImpl.java ---
    @@ -86,4 +87,8 @@ public DataMapExprWrapperImpl(TableDataMap dataMap, FilterResolverIntf expressio
       @Override public DataMapLevel getDataMapType() {
         return dataMap.getDataMapFactory().getDataMapType();
       }
    +
    +  public DataMapFactory getDataMapFactory() {
    --- End diff --
   
    this is not an interface method...can we try to get the dataMapFactory in some other way and remove this method from here?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2244#discussion_r184914866
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/blocklet/DataFileFooter.java ---
    @@ -52,7 +52,7 @@
       /**
        * Information about leaf nodes of all columns in this file
        */
    -  private List<BlockletInfo> blockletList;
    +  private transient List<BlockletInfo> blockletList;
    --- End diff --
   
    why transient is required here?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2244#discussion_r184915943
 
    --- Diff: hadoop/src/main/java/org/apache/carbondata/hadoop/CacheClient.java ---
    @@ -16,21 +16,40 @@
      */
     package org.apache.carbondata.hadoop;
     
    +import java.util.List;
    +import java.util.Map;
    +import java.util.concurrent.ConcurrentHashMap;
    +
    +import org.apache.carbondata.common.logging.LogService;
    +import org.apache.carbondata.common.logging.LogServiceFactory;
     import org.apache.carbondata.core.cache.Cache;
     import org.apache.carbondata.core.cache.CacheProvider;
     import org.apache.carbondata.core.cache.CacheType;
    +import org.apache.carbondata.core.datastore.SegmentTaskIndexStore;
     import org.apache.carbondata.core.datastore.TableSegmentUniqueIdentifier;
    +import org.apache.carbondata.core.datastore.block.SegmentProperties;
     import org.apache.carbondata.core.datastore.block.SegmentTaskIndexWrapper;
    +import org.apache.carbondata.core.metadata.AbsoluteTableIdentifier;
    +import org.apache.carbondata.core.metadata.schema.table.column.ColumnSchema;
     
     /**
      * CacheClient : Holds all the Cache access clients for Btree, Dictionary
      */
     public class CacheClient {
     
    +  private static final LogService LOGGER =
    +      LogServiceFactory.getLogService(CacheClient.class.getName());
    +
    +  private final Object lock = new Object();
    +
       // segment access client for driver LRU cache
       private CacheAccessClient<TableSegmentUniqueIdentifier, SegmentTaskIndexWrapper>
           segmentAccessClient;
     
    +  private static Map<SegmentTaskIndexStore.SegmentPropertiesWrapper, SegmentProperties>
    +      segmentProperties =
    +      new ConcurrentHashMap<>();
    --- End diff --
   
    Move this to above line


---