You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kylin.apache.org by GitBox <gi...@apache.org> on 2020/03/02 15:53:29 UTC

[GitHub] [kylin] hit-lacus opened a new pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep

hit-lacus opened a new pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-593847796
 
 
   This pull request **fixes 1 alert** when merging 892a2c2805d33de19c5e9b53cebbd39d1b71415f into 8ce3deafa1498d4f39bfb9b36e84da7627065296 - [view on LGTM.com](https://lgtm.com/projects/g/apache/kylin/rev/pr-0559a288728bb55151afcfee13b2d6f14472e21b)
   
   **fixed alerts:**
   
   * 1 for Potential input resource leak

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-594295340
 
 
   This pull request **fixes 1 alert** when merging cc063ed03ede38fe7022af6aa16bec7cffb5fe9d into 22ac0132dabff0241eb4a7781a451b82d907c5a7 - [view on LGTM.com](https://lgtm.com/projects/g/apache/kylin/rev/pr-e331435e56e933c14c749d44f58a854b31ce9d95)
   
   **fixed alerts:**
   
   * 1 for Potential input resource leak

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-595578316
 
 
   This pull request **fixes 1 alert** when merging ebe9c3817ce75af07dbcf8eb2c33e60f3a6aee9a into 541189d1e7db44821887cea40bb6e0c5a2fdf6a6 - [view on LGTM.com](https://lgtm.com/projects/g/apache/kylin/rev/pr-68e2cd904fc6c731d71c6077a174c34810954bf0)
   
   **fixed alerts:**
   
   * 1 for Potential input resource leak

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-593900437
 
 
   This pull request **fixes 1 alert** when merging 3b498893db71a3582116d0e4fad1f56bf661ed72 into 22ac0132dabff0241eb4a7781a451b82d907c5a7 - [view on LGTM.com](https://lgtm.com/projects/g/apache/kylin/rev/pr-d2eb228e519de36b49823980e348578876341497)
   
   **fixed alerts:**
   
   * 1 for Potential input resource leak

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] coveralls commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-593847096
 
 
   ## Pull Request Test Coverage Report for [Build 5686](https://coveralls.io/builds/29093517)
   
   * **0** of **4**   **(0.0%)**  changed or added relevant lines in **3** files are covered.
   * **4** unchanged lines in **2** files lost coverage.
   * Overall coverage decreased (**-0.005%**) to **27.521%**
   
   ---
   
   |  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
   | :-----|--------------|--------|---: |
   | [engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/ColumnarSplitDictReader.java](https://coveralls.io/builds/29093517/source?filename=engine-mr%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fengine%2Fmr%2Fstreaming%2FColumnarSplitDictReader.java#L71) | 0 | 1 | 0.0%
   | [engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/SaveDictStep.java](https://coveralls.io/builds/29093517/source?filename=engine-mr%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fengine%2Fmr%2Fstreaming%2FSaveDictStep.java#L139) | 0 | 1 | 0.0%
   | [engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/DictsReader.java](https://coveralls.io/builds/29093517/source?filename=engine-mr%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fengine%2Fmr%2Fstreaming%2FDictsReader.java#L54) | 0 | 2 | 0.0%
   <!-- | **Total:** | **0** | **4** | **0.0%** | -->
   
   |  Files with Coverage Reduction | New Missed Lines | % |
   | :-----|--------------|--: |
   | [core-cube/src/main/java/org/apache/kylin/cube/cuboid/TreeCuboidScheduler.java](https://coveralls.io/builds/29093517/source?filename=core-cube%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fcube%2Fcuboid%2FTreeCuboidScheduler.java#L124) | 2 | 68.46% |
   | [core-job/src/main/java/org/apache/kylin/job/impl/threadpool/DefaultScheduler.java](https://coveralls.io/builds/29093517/source?filename=core-job%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkylin%2Fjob%2Fimpl%2Fthreadpool%2FDefaultScheduler.java#L194) | 2 | 80.23% |
   <!-- | **Total:** | **4** |  | -->
   
   |  Totals | [![Coverage Status](https://coveralls.io/builds/29093517/badge)](https://coveralls.io/builds/29093517) |
   | :-- | --: |
   | Change from base [Build 5684](https://coveralls.io/builds/29091331): |  -0.005% |
   | Covered Lines: | 24294 |
   | Relevant Lines: | 88273 |
   
   ---
   ##### 💛  - [Coveralls](https://coveralls.io)
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] codecov-io commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-593489557
 
 
   # [Codecov](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=h1) Report
   > :exclamation: No coverage uploaded for pull request base (`master@7e89e95`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
   > The diff coverage is `0%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/kylin/pull/1134/graphs/tree.svg?width=650&token=JawVgbgsVo&height=150&src=pr)](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #1134   +/-   ##
   =========================================
     Coverage          ?   25.06%           
     Complexity        ?     6249           
   =========================================
     Files             ?     1446           
     Lines             ?    88273           
     Branches          ?    12351           
   =========================================
     Hits              ?    22127           
     Misses            ?    63976           
     Partials          ?     2170
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...apache/kylin/engine/mr/streaming/SaveDictStep.java](https://codecov.io/gh/apache/kylin/pull/1134/diff?src=pr&el=tree#diff-ZW5naW5lLW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reWxpbi9lbmdpbmUvbXIvc3RyZWFtaW5nL1NhdmVEaWN0U3RlcC5qYXZh) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../apache/kylin/engine/mr/streaming/DictsReader.java](https://codecov.io/gh/apache/kylin/pull/1134/diff?src=pr&el=tree#diff-ZW5naW5lLW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reWxpbi9lbmdpbmUvbXIvc3RyZWFtaW5nL0RpY3RzUmVhZGVyLmphdmE=) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [...n/engine/mr/streaming/ColumnarSplitDictReader.java](https://codecov.io/gh/apache/kylin/pull/1134/diff?src=pr&el=tree#diff-ZW5naW5lLW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reWxpbi9lbmdpbmUvbXIvc3RyZWFtaW5nL0NvbHVtbmFyU3BsaXREaWN0UmVhZGVyLmphdmE=) | `0% <0%> (ø)` | `0 <0> (?)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=footer). Last update [7e89e95...9f0029d](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] hit-lacus commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
hit-lacus commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#discussion_r388682460
 
 

 ##########
 File path: engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/SaveDictStep.java
 ##########
 @@ -108,32 +108,32 @@ public boolean accept(Path path) {
                 }
             });
 
-            SequenceFile.Reader reader;
             for (FileStatus file : files) {
-                reader = new SequenceFile.Reader(fs, file.getPath(), conf);
-                Text colName = new Text();
-                Text dictInfo = new Text();
-                while (reader.next(colName, dictInfo)) {
-                    TblColRef colRef = colRefMap.get(colName.toString());
-                    if (colRef == null) {
-                        throw new IllegalArgumentException("Invalid column name " + colName
-                                + " or it need not build dictionary!");
-                    }
-                    DictionaryInfo dictionaryInfo = serializer.deserialize(new DataInputStream(
-                            new ByteArrayInputStream(dictInfo.getBytes())));
-
-                    Dictionary dict = dictionaryInfo.getDictionaryObject();
-                    if (dict != null) {
-                        dictionaryInfo = dictManager.trySaveNewDict(dict, dictionaryInfo);
-                        cubeSeg.putDictResPath(colRef, dictionaryInfo.getResourcePath());
-                        if (cubeSeg.getRowkeyStats() != null) {
-                            cubeSeg.getRowkeyStats().add(
-                                    new Object[] { colRef.getName(), dict.getSize(), dict.getSizeOfId() });
+                try(SequenceFile.Reader reader = new SequenceFile.Reader(fs, file.getPath(), conf)) {
 
 Review comment:
   Thank you, I have updated.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] shaofengshi commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
shaofengshi commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#discussion_r387089089
 
 

 ##########
 File path: engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/SaveDictStep.java
 ##########
 @@ -136,6 +136,7 @@ public boolean accept(Path path) {
                         logger.error("dictionary of column {} not found! ", colRef.getName());
                     }
                 }
+                reader.close();
 
 Review comment:
   Will it be more safe to put it into a "finally" block?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] shaofengshi commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
shaofengshi commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#discussion_r387437755
 
 

 ##########
 File path: engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/SaveDictStep.java
 ##########
 @@ -108,32 +108,32 @@ public boolean accept(Path path) {
                 }
             });
 
-            SequenceFile.Reader reader;
             for (FileStatus file : files) {
-                reader = new SequenceFile.Reader(fs, file.getPath(), conf);
-                Text colName = new Text();
-                Text dictInfo = new Text();
-                while (reader.next(colName, dictInfo)) {
-                    TblColRef colRef = colRefMap.get(colName.toString());
-                    if (colRef == null) {
-                        throw new IllegalArgumentException("Invalid column name " + colName
-                                + " or it need not build dictionary!");
-                    }
-                    DictionaryInfo dictionaryInfo = serializer.deserialize(new DataInputStream(
-                            new ByteArrayInputStream(dictInfo.getBytes())));
-
-                    Dictionary dict = dictionaryInfo.getDictionaryObject();
-                    if (dict != null) {
-                        dictionaryInfo = dictManager.trySaveNewDict(dict, dictionaryInfo);
-                        cubeSeg.putDictResPath(colRef, dictionaryInfo.getResourcePath());
-                        if (cubeSeg.getRowkeyStats() != null) {
-                            cubeSeg.getRowkeyStats().add(
-                                    new Object[] { colRef.getName(), dict.getSize(), dict.getSizeOfId() });
+                try(SequenceFile.Reader reader = new SequenceFile.Reader(fs, file.getPath(), conf)) {
 
 Review comment:
   The code format here is not well. Please format it...

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] codecov-io edited a comment on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-593489557
 
 
   # [Codecov](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=h1) Report
   > :exclamation: No coverage uploaded for pull request base (`master@7e89e95`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
   > The diff coverage is `0%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/kylin/pull/1134/graphs/tree.svg?width=650&token=JawVgbgsVo&height=150&src=pr)](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #1134   +/-   ##
   =========================================
     Coverage          ?   25.05%           
     Complexity        ?     6246           
   =========================================
     Files             ?     1446           
     Lines             ?    88286           
     Branches          ?    12353           
   =========================================
     Hits              ?    22118           
     Misses            ?    63994           
     Partials          ?     2174
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...apache/kylin/engine/mr/streaming/SaveDictStep.java](https://codecov.io/gh/apache/kylin/pull/1134/diff?src=pr&el=tree#diff-ZW5naW5lLW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reWxpbi9lbmdpbmUvbXIvc3RyZWFtaW5nL1NhdmVEaWN0U3RlcC5qYXZh) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [.../apache/kylin/engine/mr/streaming/DictsReader.java](https://codecov.io/gh/apache/kylin/pull/1134/diff?src=pr&el=tree#diff-ZW5naW5lLW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reWxpbi9lbmdpbmUvbXIvc3RyZWFtaW5nL0RpY3RzUmVhZGVyLmphdmE=) | `0% <0%> (ø)` | `0 <0> (?)` | |
   | [...n/engine/mr/streaming/ColumnarSplitDictReader.java](https://codecov.io/gh/apache/kylin/pull/1134/diff?src=pr&el=tree#diff-ZW5naW5lLW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reWxpbi9lbmdpbmUvbXIvc3RyZWFtaW5nL0NvbHVtbmFyU3BsaXREaWN0UmVhZGVyLmphdmE=) | `0% <0%> (ø)` | `0 <0> (?)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=footer). Last update [7e89e95...cc063ed](https://codecov.io/gh/apache/kylin/pull/1134?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on issue #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#issuecomment-593496307
 
 
   This pull request **fixes 1 alert** when merging 9f0029d7edfed1080947a38ba131daacae8eca50 into 8ce3deafa1498d4f39bfb9b36e84da7627065296 - [view on LGTM.com](https://lgtm.com/projects/g/apache/kylin/rev/pr-02fc6a1dcd2004138a417e21d46910e6544218e1)
   
   **fixed alerts:**
   
   * 1 for Potential input resource leak

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] shaofengshi merged pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
shaofengshi merged pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [kylin] hit-lacus commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep

Posted by GitBox <gi...@apache.org>.
hit-lacus commented on a change in pull request #1134: KYLIN-4396 Close FileReader in SaveDictStep
URL: https://github.com/apache/kylin/pull/1134#discussion_r387407447
 
 

 ##########
 File path: engine-mr/src/main/java/org/apache/kylin/engine/mr/streaming/SaveDictStep.java
 ##########
 @@ -136,6 +136,7 @@ public boolean accept(Path path) {
                         logger.error("dictionary of column {} not found! ", colRef.getName());
                     }
                 }
+                reader.close();
 
 Review comment:
   Thank you!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services