You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/01/06 13:51:55 UTC

[GitHub] [incubator-iotdb] jixuan1989 opened a new pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk

jixuan1989 opened a new pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk
URL: https://github.com/apache/incubator-iotdb/pull/709
 
 
   when flushing a chunk, tsfile will calculate the size of the chunk and compare the size and the actual data size in the output stream.
   When calculating the size, the chunk header is calculated (which has the measurement id and therefore the size always > 0 ). However, if we find the chunk has no data points, we do not flush anything.
   In this case, the size ( >0 ) != the actual size (=0).
   
   This PR fixes the above case.

----------------------------------------------------------------
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] [incubator-iotdb] jixuan1989 commented on a change in pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk

Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on a change in pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk
URL: https://github.com/apache/incubator-iotdb/pull/709#discussion_r363853045
 
 

 ##########
 File path: tsfile/src/main/java/org/apache/iotdb/tsfile/write/chunk/ChunkWriterImpl.java
 ##########
 @@ -231,6 +231,9 @@ public long estimateMaxSeriesMemSize() {
 
   @Override
   public long getCurrentChunkSize() {
+    if (this.getCurrentDataSize() == 0) {
 
 Review comment:
   `this.getCurrentDataSize()` actually is `pageBuffer.size()` while `pageBuffer` call `outputstream.count`.
   Therefore, I remove getCurrentDataSize() and use pageBuffer.size() instead.
   
   @qiaojialin , yes I think so. 
   No matter whether you check the serialized size in flushAll... method, this method is needed and should check the data size here.

----------------------------------------------------------------
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] [incubator-iotdb] jt2594838 commented on a change in pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk

Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk
URL: https://github.com/apache/incubator-iotdb/pull/709#discussion_r363646954
 
 

 ##########
 File path: tsfile/src/main/java/org/apache/iotdb/tsfile/write/chunk/ChunkWriterImpl.java
 ##########
 @@ -231,6 +231,9 @@ public long estimateMaxSeriesMemSize() {
 
   @Override
   public long getCurrentChunkSize() {
+    if (this.getCurrentDataSize() == 0) {
 
 Review comment:
   You may save the calculation result and use it in line238 to avoid duplicated calculation.
   
   And if all chunks in a chunk group are empty, this chunk group should not be flushed into the file, which will generate an empty ChunkGroupMetadata and waste some space.

----------------------------------------------------------------
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] [incubator-iotdb] qiaojialin commented on a change in pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk

Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk
URL: https://github.com/apache/incubator-iotdb/pull/709#discussion_r363636572
 
 

 ##########
 File path: tsfile/src/main/java/org/apache/iotdb/tsfile/write/chunk/ChunkWriterImpl.java
 ##########
 @@ -231,6 +231,9 @@ public long estimateMaxSeriesMemSize() {
 
   @Override
   public long getCurrentChunkSize() {
+    if (this.getCurrentDataSize() == 0) {
 
 Review comment:
   Is this check necessary?

----------------------------------------------------------------
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] [incubator-iotdb] qiaojialin merged pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk

Posted by GitBox <gi...@apache.org>.
qiaojialin merged pull request #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk
URL: https://github.com/apache/incubator-iotdb/pull/709
 
 
   

----------------------------------------------------------------
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] [incubator-iotdb] sonarcloud[bot] commented on issue #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on issue #709: [IOTDB-401]Correct the calculation of a chunk if there is no data in the chunk
URL: https://github.com/apache/incubator-iotdb/pull/709#issuecomment-571683972
 
 
   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/B.png' alt='B' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=SECURITY_HOTSPOT) to review)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_incubator-iotdb&pullRequest=709&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo.png' alt='No Coverage information' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=709) No Coverage information  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=709&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_incubator-iotdb&pullRequest=709&metric=new_duplicated_lines_density&view=list)
   
   

----------------------------------------------------------------
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