Steven Jacobs has posted comments on this change. Change subject: Allow BAD jobs to update their specifications to use new indexes ......................................................................
Patch Set 5: (19 comments) https://asterix-gerrit.ics.uci.edu/#/c/2620/3/asterix-bad/src/main/java/org/apache/asterix/bad/BADConstants.java File asterix-bad/src/main/java/org/apache/asterix/bad/BADConstants.java: PS3, Line 45: Duration"; : String Function = "Function"; : String FIELD_NAME_ARITY = "Arity"; : String FIELD_NAME_DEPENDENCIES = "Dependencies"; : String FIELD_NAME_PARAMS = "Params"; : String FIELD_NAME_RETURN_TYPE = "ReturnType"; : String FIELD_NAME_DEFINITION = "Definition"; : String FIELD_NAME_LANGUAGE = "Language"; : String FIELD_NAME_BODY = "Body"; > It would be better to add prefixes to these names so we would know what the Given that they are "field names" and used for specific metadata, I think it's clear. Also we would have to replace one variable with two if we specified channel vs procedure. So my vote is for them to stay, but I can change it if you still think so. https://asterix-gerrit.ics.uci.edu/#/c/2620/3/asterix-bad/src/main/java/org/apache/asterix/bad/BADJobService.java File asterix-bad/src/main/java/org/apache/asterix/bad/BADJobService.java: PS3, Line 71: period, > should this be period? Done Line 91: public static boolean runDeployedJobSpecCheckPeriod(DeployedJobSpecId distributedId, IHyracksClientConnection hcc, > this name is misleading. it doesn't actually run the repetitive job but mak Done Line 115: String.valueOf(txnIdFactory.create().getId()).getBytes()); > I don't think it's a good way to do locking like this... Maybe refactor the Completely re-did this stuff to make sense Line 121: long executionMilliseconds = Instant.now().toEpochMilli() - startTime; > will this ever be true given the statements above? Completely re-did this stuff to make sense Line 210: metadataProvider.getConfig().put(ss.getPropName(), ss.getPropValue()); > same here Completely re-did this stuff to make sense Line 233: > which means the channel is actually not active? Yes, this should actually never happen but I think it's better to log a message then break the control flow if there is some crazy situation where this happens. https://asterix-gerrit.ics.uci.edu/#/c/2620/3/asterix-bad/src/main/java/org/apache/asterix/bad/lang/BADStatementExecutor.java File asterix-bad/src/main/java/org/apache/asterix/bad/lang/BADStatementExecutor.java: Line 142: private void throwErrorIfDatasetUsed(MetadataTransactionContext mdTxnCtx, String dataverse, String dataset) > change this to checkIfDatasetUsed and return true if used. Then throw an ex If we do it this way, the error message won't be as informative. The way I did it, the error says exactly what entity uses the dataset. Line 155: private void throwErrorIfFunctionUsed(MetadataTransactionContext mdTxnCtx, String dataverse, String function, > same same. Line 178: throwErrorIfDatasetUsed(mdTxnCtx, dvId, dsId.getValue()); > if (checkIfDatasetUsed(xxx)) throw new CompliationException(); same. Line 200: for (Dataverse dv : dataverseList) { > I guess you chose this it's because the channel and procedure can be much m Channels and Procedures don't directly depend on the datasets used by their function calls, but rather they depend through chaining (e.g. channel depends on function which depends on dataset). This is the way Mike recommended dependencies to be. I'm not sure if there is any smarter solution until Asterix finally has Metadata dependencies in place, in which case most of this code will be thrown out. https://asterix-gerrit.ics.uci.edu/#/c/2620/3/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 115: Pattern variableReference = Pattern.compile("([^\\w\\d])" + var.getVar() + "([^\\w\\d]|$)"); > I think you should be able to use let x = get-job-param("$x") to do the tri I think from a compilation standpoint it's better this way. I've seen lots of issues with "let" not optimizing in a good way. I know you're currently working in another branch on getting get-job-param to compile correctly, so maybe this should be visited in that branch as well? https://asterix-gerrit.ics.uci.edu/#/c/2620/3/asterix-bad/src/main/java/org/apache/asterix/bad/metadata/Channel.java File asterix-bad/src/main/java/org/apache/asterix/bad/metadata/Channel.java: Line 40: private final String channelBody; > ->channelBody? Done https://asterix-gerrit.ics.uci.edu/#/c/2620/3/asterix-bad/src/main/java/org/apache/asterix/bad/metadata/DeployedJobSpecEventListener.java File asterix-bad/src/main/java/org/apache/asterix/bad/metadata/DeployedJobSpecEventListener.java: Line 52: > what's this for? maybe add some comments? Removed this completely https://asterix-gerrit.ics.uci.edu/#/c/2620/5/asterix-bad/src/main/java/org/apache/asterix/bad/metadata/DeployedJobSpecEventListener.java File asterix-bad/src/main/java/org/apache/asterix/bad/metadata/DeployedJobSpecEventListener.java: Line 137: > rm this Done Line 169: this.state = newState; > notify all here Done PS5, Line 190: public void waitForNonSuspendedState() throws InterruptedException { : while (state == ActivityState.SUSPENDED) { : this.wait(); : } : } : : private void waitForInactiveState() throws InterruptedException { : while (isActive()) { : this.wait(); : } : } > change this to waitForState(parameter) and pass in the state. Done Line 202: public void suspend() throws HyracksDataException, InterruptedException { > method synchronized? Please beware of deadlocks. Add test unit cases for th Done Line 214: synchronized (this) { > two synchronized? Done -- To view, visit https://asterix-gerrit.ics.uci.edu/2620 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0a4d37a5b91063fcb1673dbfd008c140ed54ae6 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: Steven Jacobs <sjaco...@ucr.edu> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-HasComments: Yes
