[GitHub] carbondata pull request #2321: [WIP]clean and close datamap writers on any t...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r190922754
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/steps/DataWriterBatchProcessorStepImpl.java ---
    @@ -85,8 +86,9 @@ public DataWriterBatchProcessorStepImpl(CarbonDataLoadConfiguration configuratio
               CarbonRowBatch next = iterator.next();
               // If no rows from merge sorter, then don't create a file in fact column handler
               if (next.hasNext()) {
    +            DataMapWriterListener listener = getDataMapWriterListener(0);
                 CarbonFactDataHandlerModel model = CarbonFactDataHandlerModel
    -                .createCarbonFactDataHandlerModel(configuration, storeLocation, 0, k++);
    +                .createCarbonFactDataHandlerModel(configuration, storeLocation, 0, k++, listener);
    --- End diff --
   
    Why  not create the listener inside the `createCarbonFactDataHandlerModel`?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191123185
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/steps/DataWriterBatchProcessorStepImpl.java ---
    @@ -85,8 +86,9 @@ public DataWriterBatchProcessorStepImpl(CarbonDataLoadConfiguration configuratio
               CarbonRowBatch next = iterator.next();
               // If no rows from merge sorter, then don't create a file in fact column handler
               if (next.hasNext()) {
    +            DataMapWriterListener listener = getDataMapWriterListener(0);
                 CarbonFactDataHandlerModel model = CarbonFactDataHandlerModel
    -                .createCarbonFactDataHandlerModel(configuration, storeLocation, 0, k++);
    +                .createCarbonFactDataHandlerModel(configuration, storeLocation, 0, k++, listener);
    --- End diff --
   
    beacuse i needed listener in DataWriterProcessorStepImpl, to close all the DMwriters if the load is failed, so i created the listener outside and passed to function, which in turn will set this into model.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191148119
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/steps/DataWriterProcessorStepImpl.java ---
    @@ -84,12 +87,14 @@ public DataWriterProcessorStepImpl(CarbonDataLoadConfiguration configuration) {
         return storeLocation;
       }
     
    +
    --- End diff --
   
    remove empty line


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191148429
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/store/CarbonFactDataHandlerModel.java ---
    @@ -258,15 +258,13 @@ public static CarbonFactDataHandlerModel createCarbonFactDataHandlerModel(
         carbonFactDataHandlerModel.tableSpec = configuration.getTableSpec();
         carbonFactDataHandlerModel.sortScope = CarbonDataProcessorUtil.getSortScope(configuration);
     
    -    DataMapWriterListener listener = new DataMapWriterListener();
    -    listener.registerAllWriter(
    -        configuration.getTableSpec().getCarbonTable(),
    -        configuration.getSegmentId(),
    -        CarbonTablePath.getShardName(
    -            carbonDataFileAttributes.getTaskId(),
    -            bucketId,
    -            0,
    -            String.valueOf(carbonDataFileAttributes.getFactTimeStamp())));
    +    if (listener == null) {
    +      listener = new DataMapWriterListener();
    +      listener.registerAllWriter(configuration.getTableSpec().getCarbonTable(),
    +          configuration.getSegmentId(), CarbonTablePath
    --- End diff --
   
    change the format like original code


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191148855
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/AbstractDataLoadProcessorStep.java ---
    @@ -149,6 +153,22 @@ protected CarbonRowBatch processRowBatch(CarbonRowBatch rowBatch) {
        */
       protected abstract String getStepName();
     
    +  /**
    +   * This method registers all writer listeners and returns the listener
    +   * @param rangeId bucketId
    +   * @return
    +   */
    +  protected DataMapWriterListener getDataMapWriterListener(int rangeId) {
    +    CarbonDataFileAttributes carbonDataFileAttributes =
    +        new CarbonDataFileAttributes(Long.parseLong(configuration.getTaskNo()),
    +            (Long) configuration.getDataLoadProperty(DataLoadProcessorConstants.FACT_TIME_STAMP));
    +    DataMapWriterListener listener = new DataMapWriterListener();
    +    listener.registerAllWriter(configuration.getTableSpec().getCarbonTable(),
    --- End diff --
   
    format the code properly


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191149210
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/AbstractDataLoadProcessorStep.java ---
    @@ -149,6 +153,22 @@ protected CarbonRowBatch processRowBatch(CarbonRowBatch rowBatch) {
        */
       protected abstract String getStepName();
     
    +  /**
    +   * This method registers all writer listeners and returns the listener
    +   * @param rangeId bucketId
    +   * @return
    +   */
    +  protected DataMapWriterListener getDataMapWriterListener(int rangeId) {
    --- End diff --
   
    remove the rangeId since it is always 0


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191151271
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/AbstractDataLoadProcessorStep.java ---
    @@ -149,6 +153,22 @@ protected CarbonRowBatch processRowBatch(CarbonRowBatch rowBatch) {
        */
       protected abstract String getStepName();
     
    +  /**
    +   * This method registers all writer listeners and returns the listener
    +   * @param rangeId bucketId
    +   * @return
    +   */
    +  protected DataMapWriterListener getDataMapWriterListener(int rangeId) {
    --- End diff --
   
    it i=will be zero in all places except  in a call from DataWriterProcessorStepImpl, in processRange function, so we can keep it i think


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2321#discussion_r191156215
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/AbstractDataLoadProcessorStep.java ---
    @@ -149,6 +153,22 @@ protected CarbonRowBatch processRowBatch(CarbonRowBatch rowBatch) {
        */
       protected abstract String getStepName();
     
    +  /**
    +   * This method registers all writer listeners and returns the listener
    +   * @param rangeId bucketId
    +   * @return
    +   */
    +  protected DataMapWriterListener getDataMapWriterListener(int rangeId) {
    --- End diff --
   
    then change the parameter name to bucketId


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/2321
 
    retest sdv please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user jackylk commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---