Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )
Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name ...................................................................... Patch Set 2: (2 comments) Have a couple of questions, want to know your thoughts on them. http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java: http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45 PS2, Line 45: return Catalog.toCatalogObjectKey(toTCatalogObject()); Converting an object to it's thrift struct to get it's unique name seems kinda roundabout to me. What do you think? Also if someone wants to change the unique key for a given class, they need go through this call chain and change toCatalogObjectKey() in the Catalog class which is weird. Ideally I'd expect each CatalogObject to be able to define it's own unique name impl (override getUniqueName()) and Catalog#toCatalogObjectKey() to rely on the overriden methods. http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58 PS2, Line 58: protected abstract void writeToCatalogObject(TCatalogObject catalogObject); Rather than having 2 methods here, how about just overriding toTCatalogObject() in the base class? For example for a data source you'd do something like public final TCatalogObject toTCatalogObject() { return super.toTCatalogObject().setData_source(toThrift()); } Don't think there is a way to enforce the overrding part but that appears more cleaner (we follow that approach for toThrift() overrides) -- To view, visit http://gerrit.cloudera.org:8080/11928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f Gerrit-Change-Number: 11928 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 20 Nov 2018 22:48:35 +0000 Gerrit-HasComments: Yes
