[GitHub] carbondata pull request #2262: [CARBONDATA-2431] Incremental data added afte...

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2262#discussion_r187309701
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -424,7 +425,7 @@ public TableSegmentRefresher getTableSegmentRefresher(CarbonTable table) {
     
         // This map stores the latest segment refresh time.So in case of update/delete we check the
         // time against this map.
    -    private Map<String, Long> segmentRefreshTime = new HashMap<>();
    +    private Map<String, SegmentLatestTimestampUpdater> segmentRefreshTime = new HashMap<>();
    --- End diff --
   
    Better rename as `SegmentRefreshInfo`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2262#discussion_r187310523
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -435,23 +436,30 @@ public TableSegmentRefresher getTableSegmentRefresher(CarbonTable table) {
           SegmentUpdateDetails[] updateStatusDetails = statusManager.getUpdateStatusDetails();
           for (SegmentUpdateDetails updateDetails : updateStatusDetails) {
             UpdateVO updateVO = statusManager.getInvalidTimestampRange(updateDetails.getSegmentName());
    -        segmentRefreshTime.put(updateVO.getSegmentId(), updateVO.getCreatedOrUpdatedTimeStamp());
    +        segmentRefreshTime.put(updateVO.getSegmentId(),
    +            new SegmentLatestTimestampUpdater(updateVO.getCreatedOrUpdatedTimeStamp(), 0));
           }
         }
     
    -    public boolean isRefreshNeeded(String segmentId, SegmentUpdateStatusManager statusManager) {
    -      UpdateVO updateVO = statusManager.getInvalidTimestampRange(segmentId);
    +    public boolean isRefreshNeeded(Segment seg) throws IOException {
    +      Map<String, SegmentLatestTimestampUpdater> segmentTimestampUpdaterMap =
    --- End diff --
   
    No need to use Map here, jus store SegmentLatestTimestampUpdater is enough


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2262#discussion_r187310749
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -435,23 +436,30 @@ public TableSegmentRefresher getTableSegmentRefresher(CarbonTable table) {
           SegmentUpdateDetails[] updateStatusDetails = statusManager.getUpdateStatusDetails();
           for (SegmentUpdateDetails updateDetails : updateStatusDetails) {
             UpdateVO updateVO = statusManager.getInvalidTimestampRange(updateDetails.getSegmentName());
    -        segmentRefreshTime.put(updateVO.getSegmentId(), updateVO.getCreatedOrUpdatedTimeStamp());
    +        segmentRefreshTime.put(updateVO.getSegmentId(),
    +            new SegmentLatestTimestampUpdater(updateVO.getCreatedOrUpdatedTimeStamp(), 0));
           }
         }
     
    -    public boolean isRefreshNeeded(String segmentId, SegmentUpdateStatusManager statusManager) {
    -      UpdateVO updateVO = statusManager.getInvalidTimestampRange(segmentId);
    +    public boolean isRefreshNeeded(Segment seg) throws IOException {
    +      Map<String, SegmentLatestTimestampUpdater> segmentTimestampUpdaterMap =
    +          seg.getSegmentTimestampUpdaterMap();
    +      String segmentId = seg.getSegmentNo();
           if (segmentRefreshTime.get(segmentId) == null) {
    -        segmentRefreshTime.put(segmentId, updateVO.getCreatedOrUpdatedTimeStamp());
    +        segmentRefreshTime.put(segmentId, segmentTimestampUpdaterMap.get(segmentId));
             return true;
           }
           if (manualSegmentRefresh.get(segmentId) != null && manualSegmentRefresh.get(segmentId)) {
             manualSegmentRefresh.put(segmentId, false);
             return true;
           }
    -      Long updateTimestamp = updateVO.getLatestUpdateTimestamp();
    +      Long updateTimestamp = segmentTimestampUpdaterMap.get(segmentId).getSegmentUpdatedTimestamp();
    +
           boolean isRefresh =
    -          updateTimestamp != null && (updateTimestamp > segmentRefreshTime.get(segmentId));
    +          (updateTimestamp != null && updateTimestamp > segmentRefreshTime.get(segmentId)
    --- End diff --
   
    Move this compare logic to `SegmentLatestTimestampUpdater` compare method


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2262#discussion_r187312611
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/readcommitter/LatestFilesReadCommittedScope.java ---
    @@ -134,18 +152,29 @@ private String getSegmentID(String carbonIndexFileName, String indexFilePath) {
                   getSegmentID(carbonIndexFiles[i].getName(), carbonIndexFiles[i].getAbsolutePath());
               // TODO. During Partition table handling, place Segment File Name.
               List<String> indexList;
    +          SegmentLatestTimestampUpdater segmentLatestTimestampUpdater;
               if (indexFileStore.get(segId) == null) {
                 indexList = new ArrayList<>(1);
    +            segmentLatestTimestampUpdater =
    +                new SegmentLatestTimestampUpdater(carbonIndexFiles[i].getLastModifiedTime(), 0);
               } else {
                 // Entry is already present.
                 indexList = indexFileStore.get(segId);
    +            segmentLatestTimestampUpdater = segmentTimestampUpdaterMap.get(segId);
               }
               indexList.add(carbonIndexFiles[i].getAbsolutePath());
    +          if (segmentLatestTimestampUpdater.getSegmentUpdatedTimestamp() < carbonIndexFiles[i]
    +              .getLastModifiedTime()) {
    +            segmentLatestTimestampUpdater
    +                .setSegmentUpdatedTimestamp(carbonIndexFiles[i].getLastModifiedTime());
    +          }
               indexFileStore.put(segId, indexList);
    +          segmentLatestTimestampUpdater.setCountOfFileInSegment(indexList.size());
    +          segmentTimestampUpdaterMap.put(segId, segmentLatestTimestampUpdater);
    --- End diff --
   
    this line move to 159


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2262#discussion_r187314011
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/readcommitter/TableStatusReadCommittedScope.java ---
    @@ -77,6 +78,12 @@ public TableStatusReadCommittedScope(AbsoluteTableIdentifier identifier,
         return indexFiles;
       }
     
    +  public Map<String, SegmentLatestTimestampUpdater> getCommitedSegmentTimestampUpdaterMap(
    --- End diff --
   
    You should not return empty map here. You should pass updateManager through constructor and get the update timestamp from it and return new SegmentLatestTimestampUpdater


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2262#discussion_r187376048
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/readcommitter/TableStatusReadCommittedScope.java ---
    @@ -77,6 +78,12 @@ public TableStatusReadCommittedScope(AbsoluteTableIdentifier identifier,
         return indexFiles;
       }
     
    +  public Map<String, SegmentRefreshInfo> getCommitedSegmentTimestampUpdaterMap(
    +      Segment segment) throws IOException {
    +    Map<String, SegmentRefreshInfo> indexFiles = new HashMap<>();
    +    return indexFiles;
    --- End diff --
   
    It should return only SegmentRefreshInfo object not Map.
    And it should be fillied with updateVo timestamp.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user rahulforallp commented on the issue:

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


---