Yu-Wen Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/17538 )
Change subject: IMPALA-10724: Add mutable validWriteIdList ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2701 PS1, Line 2701: public MutableValidWriteIdList getMutableValidWriteIds() { > Can you not use the previous method itself ? The previous method is inherited from parent class Table and I shouldn't change the method signature since we only need mutable writeIdList for HdfsTable. http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java File fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java: http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@68 PS1, Line 68: public RangeResponse isWriteIdRangeValid(long minWriteId, long maxWriteId) { > Please add some unit tests for all the variations of RangeResponse. Added test in testRangeResponse http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@69 PS1, Line 69: // check the easy cases first > You can remove this comment Done http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@72 PS1, Line 72: } else if (exceptions.size() > 0 && exceptions.get(0) > maxWriteId) { > You don't need "else" Done http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@87 PS1, Line 87: } else if (count == (maxWriteId - minWriteId + 1)) { > You can remove the elses Done http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@214 PS1, Line 214: public RangeResponse isWriteIdRangeAborted(long minWriteId, long maxWriteId) { > Please add some unit tests for all the variations. Added tests in method TestRangeResponse http://gerrit.cloudera.org:8080/#/c/17538/1/fe/src/main/java/org/apache/impala/hive/common/MutableValidReaderWriteIdList.java@235 PS1, Line 235: } else if (count == (maxWriteId - minWriteId + 1)) { > Remove the elses Done -- To view, visit http://gerrit.cloudera.org:8080/17538 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28e60db0afd5d4398af24449b72abc928421f7c6 Gerrit-Change-Number: 17538 Gerrit-PatchSet: 1 Gerrit-Owner: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Reviewer: Anonymous Coward <kis...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Yu-Wen Lai <yu-wen....@cloudera.com> Gerrit-Comment-Date: Thu, 10 Jun 2021 16:42:32 +0000 Gerrit-HasComments: Yes
