Xikui Wang has posted comments on this change. Change subject: Added Procedures to BAD ......................................................................
Patch Set 5: (6 comments) I added several comments. Basically that's focusing on the EventListener and related issues. You can also discuss that with me on Skype if you find the UI here is not very convenient. 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 jobManager handles that. Check JobManager.java 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... Limited knowledge here. Just curious. :) Line 184: ActiveJobNotificationHandler.INSTANCE.monitorJob(jobId, distributedJobInfo); Do we need to monitor the distributed job here? 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. 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 you really care the about the distributedJob's status. The one that you really care is status of the one who manages the distributed job(manager?). I guess that the reason why you put the active variable for that. If I understand correctly, the EventSubscribers here didn't do much. The only use that I noticed is when createChannels, there is an assert to make sure the distributed job is up(but only for the first cycle). But everything after that, I think the subscriber is not doing anything. Correct me if I am wrong. :) 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? -- 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 <sjaco...@ucr.edu> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
