Skip to content

Conversation

mbszarek
Copy link

No description provided.

@balis balis self-requested a review November 13, 2021 19:37
@balis
Copy link
Member

balis commented Nov 13, 2021

All changes are fine except the replacement of the base image for the Hyperflow Dockerfile. This is a rather big change, can we add the autoscale plugin as a package?

Copy link
Member

@balis balis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See remarks inline


async function k8sCommand(ins, outs, context, cb) {
/** Buffer Manager configuration. */
const wfId = context.appConfig.wfId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wfId is available as context.appId (a different name is used due to historical reasons :-)). So, no need to pass it via appConfig.

async = require('async'),
eventServerFactory = require('../eventlog');
eventServerFactory = require('../eventlog'),
removeBufferManager = require('../functions/kubernetes/k8sCommand').removeBufferManager;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

engine2 is a core module, it would be best not to change it unless absolutely necessary. Creating dependency to a workflow function (which can easily be refactored out to a separate package) is not good.


Engine.prototype.workflowFinished = function() {
console.log("Workflow ["+this.wfId+"] finished. Exec trace:", this.trace+"." );
removeBufferManager(this.wfId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new part should be done outside engine2. The Engine can use eventServer to publish an event which can be received in another module. Something like eventServer.emit("trace.workflow.finished") should be okay. See various examples of usage.

COPY . /hyperflow
RUN npm install -g /hyperflow

RUN mkdir -p /tmp/kubectl && cd /tmp/kubectl && apk add curl && \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

}
});

if (wfConfig.containerSpec) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering where these containerSpec files should be published? Perhaps in workflow repositories, e.g. montage2-workflow?


async function k8sCommand(ins, outs, context, cb) {
/** Buffer Manager configuration. */
const wfId = context.appConfig.wfId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const wfId = context.appId !!! Already available!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants