Fredy Wijaya 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) 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 ki This is a fair point. Initially I thought of doing that, too. Given the current code, I can't think of an elegant way of using CatalogObject in Catalog. Let me know if you have some suggestion. 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 toTCatalogObje I know this is the convention that we in some places, however I'm kind of torn with using this style especially since there is mo safe way of enforcing that the subclass needs to call super.toTCatalogObject(). I've seen many places in Impala that uses this style and some subclasses call super and some subclasses just duplicate the code that's already been implemented in the super class. Another issue that I have with this style is there's no compile-time check that says the subclass must override toTCatalogObject(). If there's a new CatalogObject that simply extends CatalogObjectImpl without overriding toTCatalogObject(), the code will still compile fine but it will be broken at runtime. -- 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: Wed, 21 Nov 2018 02:32:03 +0000 Gerrit-HasComments: Yes
