Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/13352 )
Change subject: IMPALA-8473: publish lineage info via hook ...................................................................... Patch Set 15: (3 comments) http://gerrit.cloudera.org:8080/#/c/13352/15/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13352/15/be/src/service/impala-server.cc@505 PS15, Line 505: if (!status.ok()) { I don't see a test case for this. Looking at the code, the Hook function execution is sent into a List<Future<>> which won't be resolved because they are asynchronous. However, the return of this status is synchronous and exceptions thrown by the Future probably won't be thrown before this status is returned. I definitely think we need tests for this feature. http://gerrit.cloudera.org:8080/#/c/13352/15/fe/src/main/java/org/apache/impala/hooks/QueryExecHook.java File fe/src/main/java/org/apache/impala/hooks/QueryExecHook.java: http://gerrit.cloudera.org:8080/#/c/13352/15/fe/src/main/java/org/apache/impala/hooks/QueryExecHook.java@35 PS15, Line 35: * Any {@link Exception} thrown from this method will effectively fail Is this the proper behavior? Do we really want to prevent impala from starting up if a hook doesn't properly start? Maybe a better name for this function would be `onStartup` or `onImpalaStartup` since the function executes when startup occurs. http://gerrit.cloudera.org:8080/#/c/13352/15/fe/src/main/java/org/apache/impala/hooks/QueryExecHook.java@52 PS15, Line 52: * Any {@link Exception} thrown from this method will only be caught Aren't the exceptions from this function re-thrown in the Manager? -- To view, visit http://gerrit.cloudera.org:8080/13352 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23a896537a98bfef07fb27c70e9a87c105cd77a1 Gerrit-Change-Number: 13352 Gerrit-PatchSet: 15 Gerrit-Owner: radford nguyen <radford.ngu...@gmail.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Anonymous Coward (498) Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: radford nguyen <radford.ngu...@gmail.com> Gerrit-Comment-Date: Mon, 20 May 2019 19:08:54 +0000 Gerrit-HasComments: Yes
