[GitHub] incubator-carbondata pull request #659: Reuse the same SegmentProperties obj...

[qiuchenjian-2]

GitHub user watermen opened a pull request:

    https://github.com/apache/incubator-carbondata/pull/659

    Reuse the same SegmentProperties objects to reduce the memory

    When I load carbondata 1000+ times with 35 nodes, I found SegmentProperties occupy 2.5+G(76K * 35 * 1000) memory in driver.
    
    I don't have small files so I don't want to compact the segments. I analyzed the dump file and found the values of SegmentProperties are the same, so I think we can reuse the SegmentProperties object if possible.
   
    cc @jackylk @QiangCai

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

    $ git pull https://github.com/watermen/incubator-carbondata CARBONDATA-781

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

    https://github.com/apache/incubator-carbondata/pull/659.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 #659
   
----
commit b82e26907bd6882399cd4084e7584379b10c934c
Author: Yadong Qi <[hidden email]>
Date:   2017-03-15T09:41:42Z

    Reuse the same SegmentProperties to reduce the memory.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1168/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1176/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1186/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Earlier, this was used for handling restructure information. Now it is handled in another way, so yes we should change it to reduce the number of objects


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1200/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user watermen commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    @jackylk You mean now we can store properties in table level(Maybe called TableProperties) insteads of SegmentProperties?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1201/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user jackylk commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    I checked recently merged PR641 for restructuring, it seems that this is still used for maintaining metadata for each Segment, so it can’t be changed to TableProperties.
    One suggestion I have is to abstract and re-use some of the big object inside SegmentProperties, like the CarbonDimension and CarbonMeasure objects. They should be cached and get by ID, then it can be re-used across Segments.
   
   
    发件人: Yadong Qi [mailto:[hidden email]]
    发送时间: 2017å¹´3月17日 10:00
    收件人: apache/incubator-carbondata
    抄送: Likun (Jacky); Mention
    主题: Re: [apache/incubator-carbondata] [CARBONDATA-781] Reuse the same SegmentProperties objects to reduce the memory (#659)
   
   
    @jackylk<https://github.com/jackylk> You mean now we can store properties in table level(Maybe called TableProperties) insteads of SegmentProperties?
   
    —
    You are receiving this because you were mentioned.
    Reply to this email directly, view it on GitHub<https://github.com/apache/incubator-carbondata/pull/659#issuecomment-287245903>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGMxWr0HGumNSjzq7T693HG7dYLaiLN3ks5rmekugaJpZM4MexjG>.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user watermen commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    @jackylk If I want to reuse `SegmentProperties` and only check the parameters `columnsInTable` and `columnCardinality`, which case I will missing(wrong case)? Because compare `SegmentProperties` objects are easier than `CarbonDimension` and `CarbonMeasure` objects.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1334/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user watermen commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    @jackylk @QiangCai I have already modified code with "Store one SegmentProperties object each segment" solution.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1335/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/659#discussion_r108033699
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/block/SegmentTaskIndex.java ---
    @@ -16,30 +16,52 @@
      */
     package org.apache.carbondata.core.datastore.block;
     
    +import java.util.HashMap;
     import java.util.List;
    +import java.util.Map;
     
     import org.apache.carbondata.core.datastore.BTreeBuilderInfo;
     import org.apache.carbondata.core.datastore.BtreeBuilder;
     import org.apache.carbondata.core.datastore.impl.btree.BlockBTreeBuilder;
    +import org.apache.carbondata.core.metadata.AbsoluteTableIdentifier;
     import org.apache.carbondata.core.metadata.blocklet.DataFileFooter;
     
     /**
      * Class which is responsible for loading the b+ tree block. This class will
      * persist all the detail of a table segment
      */
     public class SegmentTaskIndex extends AbstractIndex {
    +  private static Map<SegmentKey, SegmentProperties> segmentPropertiesCached =
    --- End diff --
   
    why not use TableSegmentUniqueIdentifier?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/659#discussion_r108113194
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/block/SegmentTaskIndex.java ---
    @@ -16,30 +16,52 @@
      */
     package org.apache.carbondata.core.datastore.block;
     
    +import java.util.HashMap;
     import java.util.List;
    +import java.util.Map;
     
     import org.apache.carbondata.core.datastore.BTreeBuilderInfo;
     import org.apache.carbondata.core.datastore.BtreeBuilder;
     import org.apache.carbondata.core.datastore.impl.btree.BlockBTreeBuilder;
    +import org.apache.carbondata.core.metadata.AbsoluteTableIdentifier;
     import org.apache.carbondata.core.metadata.blocklet.DataFileFooter;
     
     /**
      * Class which is responsible for loading the b+ tree block. This class will
      * persist all the detail of a table segment
      */
     public class SegmentTaskIndex extends AbstractIndex {
    +  private static Map<SegmentKey, SegmentProperties> segmentPropertiesCached =
    --- End diff --
   
    I didn't find the class before, I will try to add a segmentproperites in TableSegmentUniqueIdentifier.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1351/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/659#discussion_r108146058
 
    --- Diff: core/src/test/java/org/apache/carbondata/core/datastore/block/SegmentTaskIndexTest.java ---
    @@ -58,7 +58,9 @@
           @Mock public void build(BTreeBuilderInfo segmentBuilderInfos) {}
         };
         long numberOfRows = 100;
    -    SegmentTaskIndex segmentTaskIndex = new SegmentTaskIndex();
    +    SegmentProperties properties = new SegmentProperties(footerList.get(0).getColumnInTable(),
    --- End diff --
   
    should be after the initialization of variable footerList.
    move to line 72


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    @watermen Attached heap dump is of executor side btree or driver side?
    1. Because in driver side btree is loaded based on segment and one segment will have only one segment property instance.
    2. In executor side carbon is loading segment property for each block(carbon data file), so number of segment properties instance will be more in executor side.
   
    In my opinion we need to optimise in executor side btree loading , for one segment across blocks there should be only one segment property instance



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user watermen commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    @kumarvishal09
    1. In the driver side, one segment has n(the number of nodes) SegmentProperties objects. You can see the `SegmentTaskIndexStore.loadBlocks` or ask @QiangCai for detail.
    2. In the executor side, we can't just remain one SegmentProperties object each segment, because tasks in same segment will run on the different executor, means different progress.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/659
 
    Build Failed  with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1356/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---