Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16159 )
Change subject: IMPALA-3127: Support incremental metadata updates in partition level ...................................................................... Patch Set 4: (13 comments) Thanks for the review! Uploaded the new patch set after it passed the exhaustive test. > I think it would be useful if we could have an exhaustive test (may be in a > separate jira) to make sure that we are not leaking partitions in statestore. > The test could add/drop partitions along with multiple add/invalidate/drop > table commands and make sure that the number of partition keys in the > statestore is as per our expectation. Yeah, created IMPALA-9994 for this. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@712 PS3, Line 712: if (!FeSupport.NativeAddPendingTopicItem(nativeCatalogServerPtr, v2Key, > Its unclear to me that when we generate the minimalObject when delete flag Sorry, this line is added in PS1 and should be removed in PS2... I add a test for this in PS4. Added these nice comments in the class comment of HdfsTable. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@742 PS3, Line 742: partObject.setId(obj.hdfs_partition.id); : } else if (obj.hdfs_partition.isSetPrev_id()) { : Preconditions.checkState( : obj.hdfs_partition.prev_id != HdfsPartition.INITIAL_PARTITION_ID - 1, : "Invalid partition id"); : > This looks a bit hacky to me. Do you think it would be more readable by add I think this way satifies the meaning of invalidations better. LocalCatalog coordinators don't need to distinguish whether an invalidation is an "update" invalidation or a "delete" invalidation. On the other hand, catalogd sends minimal objects as invalidations because it knows the implementation of coordinators. I think it's ok for adding the awareness of how coordinator use the partition ids. BTW, the prev_id field is added in THdfsPartition but is only used in passing the previous partition id through here. I'll define its default value to -1 in thrift definition. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1295 PS3, Line 1295: topicUpdateEntry.getLastSentVersion(), > wouldn't this line be called for both fullUpdate and a incremental update? Sorry, I think I use "incremental updates" in many places and it introduce confusions. toThriftWithPartitionIds() is used when catalogd wants to send partition updates individually instead of carrying them inside the thrift table. I call this "incremental updates" but I think I should avoid the conflicts with incremental catalog topic updates. Will update the javadoc. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1321 PS3, Line 1321: // statestored restarts). : if (ctx.isFullUpdate()) hdfsTable.resetMaxSentPartitionId(); : > nit, perhaps this is more readable? Done http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1329 PS3, Line 1329: > Can you clarify why this is needed only in case of incremental updates? Wha Sorry, I thought no one can't make use of these in a full topic update. But it's only true for statetore and v1 coordinators. When statestore restarts, its catalog topic map is empty. It will fetch a full topic update (fromVersion=0) from catalogd. But there are no old values to be reset in its catalog topic map. When statestore restarts, V1 coordinators will receive a full topic update which will trigger it to reset the whole local cache. They don't need deletions in the new empty cache. Actually, partition deletions are always ignored by v1 coordinators since partition deletions are detected by absense of the id in table's latest partition list. However, v2 coordinators won't reset the cache so they can still use them to invalidate obsolete partition cache. Will remove this check. http://gerrit.cloudera.org:8080/#/c/16159/3/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/16159/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@90 PS3, Line 90: ly instead of > I think it is worth documenting that even though this extends CatalogObject Sure. Done. http://gerrit.cloudera.org:8080/#/c/16159/3/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/16159/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@273 PS3, Line 273: on for > nit, replace with "known" instead of "cached" since local coordinators don' Done http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@278 PS3, Line 278: e that this t > nit, Do we need to store the full HdfsPartition if it is dropped? Seems lik Yeah, done. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@208 PS3, Line 208: Map<TableName, List<THdfsPartition>> newPartitionsByTable = new HashMap<>(); > Instead of tracking the partitions separately in this map do you think it w The problem is that we should update the table in an atomic way so queries won't see partial tables. When addTable() is called, we should be able to provide a new and complete table object which requires applying the partition updates. So applying the partiton changes in addTable() is easier to atomically update the table. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@466 PS3, Line 466: h minimal : // partitions. Currently, catalogd returns full tabl > can you please update this comment? Done http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@492 PS3, Line 492: HdfsPartition tPart : newPartitions) { > Why is this not a Preconditions.checkState? Done http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1378 PS3, Line 1378: obj.hd > nit, may be replace with "obsolete"? Remove this comment since we send invalidation for updates as well now. http://gerrit.cloudera.org:8080/#/c/16159/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1451 PS3, Line 1451: > nit, may be more helpful to print the partitionNames here instead of id? Al Done. Changed to "partition dbName.tblName:partitionName (id=$id)" -- To view, visit http://gerrit.cloudera.org:8080/16159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0abfb346903d6e7cdc603af91c2b8937d24d870 Gerrit-Change-Number: 16159 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <[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: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 22 Jul 2020 22:53:59 +0000 Gerrit-HasComments: Yes
