Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13049 )
Change subject: IMPALA-8149 : Add support for alter_database events ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/main/java/org/apache/impala/catalog/Db.java@523 PS4, Line 523: + " its max capacity %d. Ignoring add request for version number %d. This " : + "could cause unnecessary database invalidation when the event is " : nit: fix indentation, 4 spaces for continued indentation http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@439 PS1, Line 439: protected void initSelfEventIdentifiersFromEvent() { : throw new UnsupportedOperationException("Please override this method in subclass"); : } > I tried to make it abstract, but some subclasses don't need call initSelfEv In general, it's always a good idea to force the implementers to make a conscious decision whether or not to override for compile-time safety rather than forgetting to override the method and get a runtime exception. However, I have no string opinion on this since the subclasses are isolated. Perhaps, we can rename the exception message to use: throw new UnsupportedOperationException(String.format( "%s is not supported", ClassUtil.getMethodName())); http://gerrit.cloudera.org:8080/#/c/13049/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@445 PS1, Line 445: return params.getOr > So shall I remove this check? As empty case is handled by next line getOrDe We can just leave it for now. Removing the check is wrong if params can be null and L446 will throw an NPE. http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/13049/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@343 PS4, Line 343: nit: fix indentation -- To view, visit http://gerrit.cloudera.org:8080/13049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf020e85cae04163bf32e31363eb4119d624640b Gerrit-Change-Number: 13049 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward <xiaom...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <xiaom...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Apr 2019 17:36:37 +0000 Gerrit-HasComments: Yes
