Xikui Wang has posted comments on this change. Change subject: Allow BAD jobs to update their specifications to use new indexes ......................................................................
Patch Set 3: (14 comments) I've added a couple of comments to your change. Let me know if you want to talk over them on skype. 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 they are actually for in the future... Like FunctionBody or ChannelBody... It seems it requires a metadata change which is not backward compatible? It's up to you... 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: duration should this be period? Line 91: public static boolean runRepetitiveDeployedJobSpec(DeployedJobSpecId distributedId, IHyracksClientConnection hcc, this name is misleading. it doesn't actually run the repetitive job but makes sure the execution time under period requirement right? change it to "doDeployedJobWithPeriodRequirement" or something similar? Line 115: success = listener.setInstanceCount(DeployedJobSpecEventListener.InstanceChange.INCREASE); I don't think it's a good way to do locking like this... Maybe refactor the listener to use wait and notify? Line 121: if (!success) { will this ever be true given the statements above? Line 210: if (success) { same here Line 233: LOGGER.severe("Tried to redeploy the job for " + entityId + " but no listener exists."); which means the channel is actually not active? 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 exception after the function call if the return value is true... Line 155: private void throwErrorIfFunctionUsed(MetadataTransactionContext mdTxnCtx, String dataverse, String function, same Line 178: throwErrorIfDatasetUsed(mdTxnCtx, dvId, dsId.getValue()); if (checkIfDatasetUsed(xxx)) throw new CompliationException(); Line 200: for (Dataverse dv : dataverseList) { I guess you chose this it's because the channel and procedure can be much more than the functions? I think it only matters if the channel or procedure is deployed. Can we do something smarter here? 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 trick? 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 body; ->channelBody? 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: LOCK, what's this for? maybe add some comments? -- 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: 3 Gerrit-Project: asterixdb-bad Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-HasComments: Yes
