Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/8825 )
Change subject: IMPALA-5990: End-to-end compression of metadata ...................................................................... Patch Set 9: (5 comments) Great job! http://gerrit.cloudera.org:8080/#/c/8825/9/be/src/catalog/catalog-util.h File be/src/catalog/catalog-util.h: http://gerrit.cloudera.org:8080/#/c/8825/9/be/src/catalog/catalog-util.h@81 PS9, Line 81: explicit Why do you need that? http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/CatalogInternalService.thrift File common/thrift/CatalogInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/CatalogInternalService.thrift@30 PS9, Line 30: NativeAddTopicUpdate() Did you mean AddPendingTopicItem? I couldn't find this function. http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/CatalogInternalService.thrift@41 PS9, Line 41: / List of updated (new and modified) catalog objects whose catalog verion is : // larger than TGetCatalotDeltaRequest.from_version. Deprecated after IMPALA-5990. : 2: optional list<CatalogObjects.TCatalogObject> updated_objects_deprecated : : // List of deleted catalog objects whose catalog version is larger than : // TGetCatalogDelta.from_version. Deprecated after IMPALA-5990. : 3: optional list<CatalogObjects.TCatalogObject> deleted_objects_deprecated Why keeping them around? http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/8825/9/common/thrift/Frontend.thrift@668 PS9, Line 668: // New or modified items. Empty list if no items were updated. Deprecated after : // IMPALA-5990. : 3: optional list<CatalogObjects.TCatalogObject> updated_objects_deprecated : : // Empty if no items were removed or is_delta is false. Deprecated after IMPALA-5990. : 4: optional list<CatalogObjects.TCatalogObject> removed_objects_deprecated Same here. Do we need to keep them around? http://gerrit.cloudera.org:8080/#/c/8825/9/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/8825/9/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@29 PS9, Line 29: import org.apache.impala.thrift.*; We typically avoid the wildcard import. thrift.* has lot's of stuff you currently don't need. -- To view, visit http://gerrit.cloudera.org:8080/8825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae Gerrit-Change-Number: 8825 Gerrit-PatchSet: 9 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Wed, 31 Jan 2018 22:46:50 +0000 Gerrit-HasComments: Yes
