Xikui Wang has posted comments on this change. Change subject: Move channels to a result sharing framework ......................................................................
Patch Set 1: (18 comments) added some comments. https://asterix-gerrit.ics.uci.edu/#/c/2731/1/asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/ChannelSubscribeStatement.java File asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/ChannelSubscribeStatement.java: Line 181: StringBuilder builder = new StringBuilder(); One side comment. I think these constructed stmts are one per query. if not, maybe you want to change it accordingly, as you've heard from them that multiple stmts in one query will soon become invalid. Line 182: builder.append("upsert into " + channelSubscriptionsDataset + "(\n"); quick Q. This works without use dataverse? PS1, Line 183: s Change this a constant and give it a better name. PS1, Line 185: param also, is this part of certain meta data schema? PS1, Line 185: param Change it to a constant variable. PS1, Line 195: param same PS1, Line 229: param same PS1, Line 249: s same here https://asterix-gerrit.ics.uci.edu/#/c/2731/1/asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/CreateChannelStatement.java File asterix-bad/src/main/java/org/apache/asterix/bad/lang/statement/CreateChannelStatement.java: PS1, Line 184: partitionFields = new ArrayList<>(); : keyIndicators = new ArrayList<>(); : keyIndicators.add(0); : keyIndicators.add(0); : fieldName = new ArrayList<>(); : List<String> fieldName2 = new ArrayList<>(); : fieldName.add(BADConstants.ChannelSubscriptionId); : fieldName2.add(BADConstants.BrokerSubscriptionId); : partitionFields.add(fieldName); : partitionFields.add(fieldName2); Do you mind explain this a little bit? also, regular_name+number is very confusing... PS1, Line 280: a I'm ok with constructing query strings. One minor suggestion would be to create constant variables for variable names that are used in the query. There are several benefits: 1) easier for future maintenance; 2) If the variable means something, it can be shown in the names; 3) less likely to have conflicts when adding new variables. https://asterix-gerrit.ics.uci.edu/#/c/2731/1/asterix-bad/src/main/java/org/apache/asterix/bad/rules/InsertBrokerNotifierForChannelRule.java File asterix-bad/src/main/java/org/apache/asterix/bad/rules/InsertBrokerNotifierForChannelRule.java: Line 186: //Maybe we need to add a project??? ? Line 245: empty line Line 343: /*This function is used to find specific operators within the plan, either You should be able to use the function signature to get a better template for this. PS1, Line 352: String param1, String param2 these two parameters seem not being used except for in recursive call... safely to remove? Line 352: private AbstractLogicalOperator findOp(AbstractLogicalOperator op, int searchId, String param1, String param2) { I think you could use LogicalOperatorTag here directly https://asterix-gerrit.ics.uci.edu/#/c/2731/1/asterix-bad/src/test/resources/runtimets/queries/channel/room_occupants/room_occupants.4.query.sqlpp File asterix-bad/src/test/resources/runtimets/queries/channel/room_occupants/room_occupants.4.query.sqlpp: PS1, Line 30: param0 correlation with the createChannel param0? (probably not...) https://asterix-gerrit.ics.uci.edu/#/c/2731/1/asterix-bad/src/test/resources/runtimets/queries/channel/shared_subscriptions/shared_subscriptions.3.query.sqlpp File asterix-bad/src/test/resources/runtimets/queries/channel/shared_subscriptions/shared_subscriptions.3.query.sqlpp: PS1, Line 21: param0 same here? https://asterix-gerrit.ics.uci.edu/#/c/2731/1/asterix-bad/src/test/resources/runtimets/queries/channel/shared_subscriptions/shared_subscriptions.6.pollquery.sqlpp File asterix-bad/src/test/resources/runtimets/queries/channel/shared_subscriptions/shared_subscriptions.6.pollquery.sqlpp: Line 19: // polltimeoutsecs=15 ws -- To view, visit https://asterix-gerrit.ics.uci.edu/2731 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifbcdf264bcd21caa0d28a9ac392b36577ca60dad Gerrit-PatchSet: 1 Gerrit-Project: asterixdb-bad Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <sjaco...@ucr.edu> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-HasComments: Yes
