Steven Jacobs has posted comments on this change. Change subject: Added Procedures to BAD ......................................................................
Patch Set 5: (6 comments) I eliminated bad calls for the event listeners and minimized the functionality being used by PrecompiledJobEventListener. I think we will still need to talk more about refactoring this class. Can you check out the rest of the changes in the new patchset and we can focus on that file last? https://asterix-gerrit.ics.uci.edu/#/c/1473/5/asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/ChannelDropStatement.java File asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/ChannelDropStatement.java: Line 122: ActiveLifecycleListener.INSTANCE.notifyJobFinish(hyracksJobId); > I don't think you need to notifyJobFinish here explicitly. It seems the job Done https://asterix-gerrit.ics.uci.edu/#/c/1473/5/asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/CreateProcedureStatement.java File asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/CreateProcedureStatement.java: Line 142: duration = ((StringLiteral) ((LiteralExpr) period.getExprList().get(0)).getValue()).getValue(); > Is there a better/simpler way to get this parameter? Not sure though... Lim I don't think so. The problem is that for Parser sake we just want to use a call expression, but it is actually a very specific case that we actually use (a call to duration). Line 184: ActiveJobNotificationHandler.INSTANCE.monitorJob(jobId, distributedJobInfo); > Do we need to monitor the distributed job here? Done https://asterix-gerrit.ics.uci.edu/#/c/1473/5/asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/ProcedureDropStatement.java File asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/ProcedureDropStatement.java: Line 118: ActiveLifecycleListener.INSTANCE.notifyJobFinish(hyracksJobId); > Same here. Probably no need to notifyJobFinish explicitly. Done https://asterix-gerrit.ics.uci.edu/#/c/1473/5/asterix-bad/src/main/java/org/apache/asterix/bad/metadata/PrecompiledJobEventListener.java File asterix-bad/src/main/java/org/apache/asterix/bad/metadata/PrecompiledJobEventListener.java: Line 147: notifyEventSubscribers(event); > IMO, managing the ActiveLifecycleEvent is not necessary here. I don't think Let's talk more about this class in the next patchset. Take a look through my changes to the rest of the code and see what you think. Then we can focus on redoing this class last (since a lot of it will need to be removed/changed) https://asterix-gerrit.ics.uci.edu/#/c/1473/5/asterix-bad/src/test/resources/runtimets/queries/procedure/insert_procedure/insert_procedure.1.ddl.aql File asterix-bad/src/test/resources/runtimets/queries/procedure/insert_procedure/insert_procedure.1.ddl.aql: > Maybe rename this test case to create_procedure? Name Confusion :) This isn't a test to "insert a procedure" but rather to test a procedure who's job is an insert job -- To view, visit https://asterix-gerrit.ics.uci.edu/1473 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03550a74e2c90179e72345103b3d2c4f98148631 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb-bad Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
