Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17505 )
Change subject: IMPALA-7501: Slim down partition metadata in LocalCatalog mode ...................................................................... Patch Set 4: (7 comments) Thank Aman's feedbacks! http://gerrit.cloudera.org:8080/#/c/17505/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17505/3//COMMIT_MSG@7 PS3, Line 7: meta > nit: did you mean metadata ? (meta has other meanings). Yeah, not aware of other meanings.. I think we use 'meta' as 'metadata' in many places, i.e. CatalogdMetaProvider, DirectMetaProvider, TBriefTableMeta, TableMetaRef, etc. Anyway, I'll try to use 'metadata' if 'meta' causes confusion. http://gerrit.cloudera.org:8080/#/c/17505/3//COMMIT_MSG@11 PS3, Line 11: used a > nit: use and (remove the comma) Done http://gerrit.cloudera.org:8080/#/c/17505/3//COMMIT_MSG@11 PS3, Line 11: Mos > change to '..suboptimal. Most of them..' Done http://gerrit.cloudera.org:8080/#/c/17505/3//COMMIT_MSG@27 PS3, Line 27: meta > nit: metadata Done http://gerrit.cloudera.org:8080/#/c/17505/3/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/17505/3/common/thrift/CatalogObjects.thrift@325 PS3, Line 325: // These are Literal expressions > Any reason why these commented fields cannot be removed ? I think we keep the conversion that when removing thrift fields, only commenting them out in case the field ids are occasionally used by other usages, e.g. https://github.com/apache/impala/blob/822e8373d1f1737865899b80862c2be7b07cc950/common/thrift/BackendGflags.thrift#L47 EDIT: However, I think we can just remove them since this thrift object is invisible for user/clients. So we don't need to take care of compability, i.e. we don't guarantee that different versions of catalogd and coordinator can work together. Thanks for raising this question! http://gerrit.cloudera.org:8080/#/c/17505/3/common/thrift/CatalogObjects.thrift@332 PS3, Line 332: // a partition which will be created as part of a query. > Since this is number 27, could you move it down to line 396 ? The un-ordere Sure. I left them here to indicate that the above removed fields are replaced by it. I can move it down since we decide to remove the above comments. http://gerrit.cloudera.org:8080/#/c/17505/3/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/17505/3/common/thrift/CatalogService.thrift@401 PS3, Line 401: 11: optional i64 write_id > Does the write_id need to be sent for each partition ? This is the last com Good point. Maybe we don't need it in coordinator side. I'll check deeper. I just extract it from hive_metastore.Partition: https://github.com/apache/hive/blob/b1929f30931adbed50a7387d49fdcc72ac945e3a/standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift#L610 ... and use it to serve LocalFsPartition#getWriteId(). So before this patch, we always send it inside the hms Partition object. Hopefully, we can remove it if the coordinator don't use it. I'll address this in a later patch. -- To view, visit http://gerrit.cloudera.org:8080/17505 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I307e7a8193b54a7b3ab93d9ebd194766bbdbd977 Gerrit-Change-Number: 17505 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Fri, 28 May 2021 13:39:32 +0000 Gerrit-HasComments: Yes
