Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15985 )
Change subject: IMPALA-9778: Refactor HdfsPartition to be immutable ...................................................................... Patch Set 10: (16 comments) http://gerrit.cloudera.org:8080/#/c/15985/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15985/10//COMMIT_MSG@13 PS10, Line 13: - Aborted DDL/DMLs won't leave partition metadata in a bad shape, which Are there specific bugs that are fixed by this patch? Or it this a class of potential bugs that are now impossible? http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@519 PS10, Line 519: public final List<String> sdBucketCols; Wonder if we should change these to ImmutableList/ImmutableMap so that it's explicit that they're immutable. http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@564 PS10, Line 564: sdBucketCols = other.sdBucketCols; Comment that we can safely reuse these because they're immutable. Maybe not needed if we make the types ImmutableList/ImmutableMap http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@589 PS10, Line 589: private final static AtomicLong partitionIdCounter_ = new AtomicLong(); Maybe move this static value to a different block, e.g. above 585. It's a bit weird to have it mixed in with the instance variables. http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@627 PS10, Line 627: Populated : // when the partition is loaded and updated using setPartitionStatsBytes(). I think this needs to get updated - we should not be mutating this with setPartitionStatsBytes(). http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@967 PS10, Line 967: public HdfsPartition build() { Is it valid to continue using the Builder after build() is called? http://gerrit.cloudera.org:8080/#/c/15985/10/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/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@444 PS10, Line 444: public boolean isDirtyPartition(HdfsPartition partition) { nit: we usually leave blank lines in-between multi-line methods. http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@447 PS10, Line 447: public HdfsPartition.Builder pickInprogressPartitionBuilder(HdfsPartition partition) { Might help to briefly comment these methods, e.g. this one should document that it marks the partition as non-dirty http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@455 PS10, Line 455: public boolean hasInProgressModification() { return !dirtyPartitions_.isEmpty(); } Probably worth adding a brief comment, e.g @returns true if any partitions are dirty http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@803 PS10, Line 803: private HdfsPartition.Builder createPartition(StorageDescriptor storageDescriptor, Maybe update to createPartitionBuilder? http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@897 PS10, Line 897: } nit: add a blank line between methods. http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@945 PS10, Line 945: // dirtyPartitions_ is only maintained in the catalogd. nullPartitionIds_ and We should document this in the comments for these variables. http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1428 PS10, Line 1428: * 'partitionNames' and adds them to the internal list of table partitions. Can you document inprogressPartBuilders? http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@2062 PS10, Line 2062: This is one step towards eventually implementing IMPALA-7533. Maybe we can remove this reference, since we are always doing this now. http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/catalog/Table.java@802 PS10, Line 802: public boolean hasInProgressModification() { return false; } These methods deserve a short comment - how they fit into the execution of a catalog operation, and how they are used (it looks like they are currently just used for assertions to make sure that the table doesn't end up in an inconsistent state at the end of an operation). http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/15985/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@849 PS10, Line 849: modificaiton typo: modification -- To view, visit http://gerrit.cloudera.org:8080/15985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib52e5810d01d5e0c910daacb9c98977426d3914c Gerrit-Change-Number: 15985 Gerrit-PatchSet: 10 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Fri, 05 Jun 2020 21:39:01 +0000 Gerrit-HasComments: Yes
