Enable alternative messaging between master and worker #682

nklincoln · 2019-12-09T15:36:52Z

Rather large PR this one 😂

This PR modularises the messaging between the master orchestrator and workers that interact with a SUT:

Default remains as process based communications
MQTT added as a different communication protocol

Additionally:

Adds a prepare phase that is responsible for running the callback init function
client renamed with 'worker'
workerId assigned within initial communication handshake
phasing of test(s) made more obvious, with new promise resolution method 'updateClientPhase' and explicit ASSIGNED/READY/PREPARED phases

Integration tests modified to test Fabric using mqtt communications, other SUT tests default to the process based communications. (fabric log level reduced to info, as log size was being exceeded in builds)

Signed-off-by: nkl199@yahoo.co.uk nkl199@yahoo.co.uk

- Added messaging capability with default mqtt comms - client replaced with 'worker' and rename of relevant files - clientId assigned by client factory spawn process - clientId held internal to adaptor - blockchain prepareClients method changed to prepareWorkerArguments - prepare phase added to orchestrator to run the callback `init` function as a distinct and unmonitored step - phasing of test made more obvious, with new promise resolution method 'updateClientPhase' and explicit READY, PREPARED, TEST phases Signed-off-by: nkl199@yahoo.co.uk <nkl199@yahoo.co.uk>

aklenik

Superb PR 🎆 🥇
Almost all of the remarks are general insights, and up for debate. The only "bug" is in the Ethereum adapter's prepareWorkerArguments function, but even that is harmless, can be fixed later, so I'm approving the PR as is. This way the depending Fabric adapter PR doesn't need to be rebased.

aklenik · 2020-01-14T22:22:54Z

packages/caliper-ethereum/lib/ethereum.js

        let result = [];
-        for (let i = 0 ; i< number ; i++) {
+        for (let i = 0 ; i<= number ; i++) {


The i <= number condition results in number + 1 array items. Which conflicts with the default BlockchainInterface implementation.

aklenik · 2020-01-14T22:24:27Z

packages/caliper-fabric/lib/fabric.js

@@ -2212,8 +2211,7 @@ class Fabric extends BlockchainInterface {

            // We are done - return the networkUtil object
            return {
-                networkInfo: this.networkUtil,
-                clientIdx
+                networkInfo: this.networkUtil


Shouldn't we also return the worker index in the context? The Ethereum adapter does that.

aklenik · 2020-01-14T22:24:56Z

packages/caliper-fabric/lib/fabric.js

@@ -2291,8 +2289,7 @@ class Fabric extends BlockchainInterface {
            }

            return {
-                networkInfo: this.networkUtil,
-                clientIdx
+                networkInfo: this.networkUtil


Return the worker index here too.

aklenik · 2020-01-14T22:30:13Z

packages/caliper-fisco-bcos/lib/fiscoBcos.js

     * @return {Promise<object>} The promise for the result of the execution.
     */
-    async getContext(name, args, clientIdx) {
+    async getContext(name, args) {


Maybe we should return the worker index for consistency since that's a SUT-independent data.
(Might be worth implementing this at the interface level, so it's always included.)

aklenik · 2020-01-14T22:34:29Z

packages/caliper-iroha/lib/iroha.js

     * @return {object} The assembled Iroha context.
     * @async
     */
-    async getContext(name, args, clientIdx, txFile) {
+    async getContext(name, args) {


Should include worker index in returned context (easier at the interface level, this is just another reminder here).

aklenik · 2020-01-14T22:36:44Z

packages/caliper-sawtooth/lib/sawtooth.js

        super(config_path);
        configPath = config_path;
        this.bcType = 'sawtooth';
        this.workspaceRoot = workspace_root;
+        this.clientIndex = clientIndex;


Another reminder to include the worker index in the context :)

aklenik · 2020-01-14T22:54:19Z

packages/caliper-core/lib/common/messaging/messenger-interface.js

+     * Configure the Messenger for use
+     * @async
+     */
+    async configure() {


The configurationObject parameter used in derived classes is missing from the interface.

aklenik · 2020-01-14T22:58:10Z

packages/caliper-core/lib/common/messaging/messenger.js

+        const message = {
+            to,
+            from: this.messenger.getUUID(),
+            timestamp: date.toISOString(),


The epoch ms returned by Date.now() might be more general and easier to handle across different systems.

aklenik · 2020-01-14T23:14:23Z

packages/caliper-core/lib/common/messaging/mqtt-worker.js

+/**
+ * Messenger that is based on an mqtt implementation
+ */
+class MqttWorkerMessenger extends MessengerInterface {


A lot of similarities with the master mqtt messenger. Might worth investigating later how to unify things.

aklenik · 2020-01-14T23:22:16Z

packages/caliper-core/lib/common/messaging/mqtt-worker.js

+     * Configure the Messenger for use
+     * @param {MessageHandler} handlerContext a configured message handler
+     */
+    configure(handlerContext) {


Comparing to the master's configure method, they behave quite differently.
I'm not sure whether the logical type of the message (worker update, or some phase-related message) should matter at this point. Maybe both should just forward the decoded messages to a message handler instance, and the underlying instances would differentiate between the valid message types (like intended for workers or for master). This would make unifying the two mqtt handlers simpler.

nklincoln requested a review from aklenik December 9, 2019 15:36

nklincoln requested review from feihujiang, jiangyaoguo, liam-grace and panyu4 as code owners December 9, 2019 15:36

nklincoln force-pushed the client-mods-mqtt branch 2 times, most recently from d192a39 to 82e6d8c Compare December 11, 2019 11:11

aklenik mentioned this pull request Jan 8, 2020

Make logging consistent #536

Open

4 tasks

aklenik self-assigned this Jan 8, 2020

aklenik added the PR/under review The PR is currently under review label Jan 8, 2020

nklincoln force-pushed the client-mods-mqtt branch 2 times, most recently from effe9e5 to 4367e33 Compare January 10, 2020 16:44

nklincoln force-pushed the client-mods-mqtt branch from 4367e33 to d6ef2cf Compare January 12, 2020 07:11

nklincoln mentioned this pull request Jan 12, 2020

[Built on top of messenger PR] Split Fabric Adaptors and enable Fabric v2 #703

Merged

aklenik approved these changes Jan 15, 2020

View reviewed changes

aklenik removed the PR/under review The PR is currently under review label Jan 15, 2020

nklincoln merged commit 32c0e77 into hyperledger:master Jan 15, 2020

nklincoln mentioned this pull request Jan 15, 2020

When creating resources within an 'init' stage, can break txNumber test #570

Closed

nklincoln deleted the client-mods-mqtt branch January 23, 2020 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable alternative messaging between master and worker #682

Enable alternative messaging between master and worker #682

nklincoln commented Dec 9, 2019

aklenik left a comment

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

aklenik Jan 14, 2020

Enable alternative messaging between master and worker #682

Enable alternative messaging between master and worker #682

Conversation

nklincoln commented Dec 9, 2019

aklenik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment