kis...@cloudera.com 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 ? 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. 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 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" 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 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. 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 -- 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-Comment-Date: Wed, 09 Jun 2021 21:28:49 +0000 Gerrit-HasComments: Yes
