Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide cli command to spawn a distributed worker #734

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

nklincoln
Copy link
Contributor

CLI command to spawn a worker that uses a non-process comms method

Changes include:

  • enable passage of yaml file to override internal default bind options
  • worker spawn command conditioned on:
    • must be remote
    • must not use process based comms
  • worker spawn includes calling of existing bind command
  • Conversion of Worker factory to be a static method
  • Rename of factories and sut workers from "client"

closes #697
Signed-off-by: [email protected] [email protected]

@nklincoln nklincoln force-pushed the spawn-client-cli branch 2 times, most recently from 7b2a147 to bb149ad Compare February 19, 2020 11:30
@aklenik aklenik self-assigned this Feb 19, 2020
Copy link
Contributor

@aklenik aklenik left a comment

Choose a reason for hiding this comment

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

See the comments in the review. Nothing major, overall LGTM 👍

packages/caliper-cli/caliper.js Outdated Show resolved Hide resolved
packages/caliper-cli/lib/bind/bind.js Outdated Show resolved Hide resolved

let sutList = Object.keys(bindOptions.sut);
if (!sut) {
let msg = `SUT name is not specified. Available SUTs: ${sutList.join(' | ')}`;
let msg = `SUT name is not specified. Available SUTs: ${Object.keys(defaultBindOpts.sut).join(' | ')}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we list the SUT names available in the set config file (not necessarily the default)?
So ${Object.keys(bindOptions.sut).join(' | ')} or simply ${sutList.join(' | ')}
Same for the other error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was an attempt to try and protect us from someone passing in an incorrect file - if it is completely wrong, then we won't be able to list anything.

Which raises the question on the need for a validator here

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, a validator is definitely needed. Then let's leave it as it is for now.

});
yargs.usage('Usage:\n caliper bind --caliper-bind-sut fabric --caliper-bind-sdk 1.4.1 --caliper-bind-cwd ./ --caliper-bind-args="-g"');

// enforce the option after these options
yargs.requiresArg(['caliper-bind-sut','caliper-bind-sdk','caliper-bind-args', 'caliper-bind-cwd']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, the requiresArg call has been removed from our run command, so the options can also be specified from other sources (env, config file, etc). Strange how this line didn't cause a problem so far....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the above are the required items as per the bind configuration only - which iirc also complains if the items are not passed. But you are correct - this is essentially a run command, so we should map to that pattern and only throw/reject if there are no values at the point of use

packages/caliper-cli/lib/worker/lib/launch.js Outdated Show resolved Hide resolved
@nklincoln nklincoln force-pushed the spawn-client-cli branch 4 times, most recently from 9dfbec4 to 69572d4 Compare February 20, 2020 09:00
Copy link
Contributor

@aklenik aklenik left a comment

Choose a reason for hiding this comment

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

Just the minor strict duplicate in the CLI, then good to go 👍

packages/caliper-cli/caliper.js Outdated Show resolved Hide resolved
@aklenik aklenik merged commit 9bf61aa into hyperledger-caliper:master Feb 20, 2020
@nklincoln nklincoln deleted the spawn-client-cli branch April 8, 2020 17:24
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.

Extend Caliper CLI
2 participants