New `web-sockets` connector #5170

miiu96 · 2023-04-11T14:39:19Z

Reasoning behind the pull request

Refactored the WebSocket connector

Proposed changes

Moved the WebSocketDriver ( now named `HostDriver ) in this repository.
Integrate the FullDuplexHost from mx-chain-communication-go repository (https://github.com/multiversx/mx-chain-communication-go).

Testing procedure

Verify that all data are indexed in the Elasticseach cluster

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

codecov · 2023-04-11T14:53:29Z

Codecov Report

Patch coverage: 60.33% and no project coverage change.

Comparison is base (3b17089) 70.82% compared to head (3df9a57) 70.82%.

❗ Current head 3df9a57 differs from pull request most recent head a58411d. Consider uploading reports for the commit a58411d to get more accurate results

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feat/outport-refactor    #5170   +/-   ##
======================================================
  Coverage                  70.82%   70.82%           
======================================================
  Files                        675      677    +2     
  Lines                      87850    87941   +91     
======================================================
+ Hits                       62218    62284   +66     
- Misses                     20940    20960   +20     
- Partials                    4692     4697    +5

Impacted Files	Coverage Δ
facade/initial/initialNodeFacade.go	`73.33% <0.00%> (-1.00%)`	⬇️
facade/nodeFacade.go	`78.35% <0.00%> (-0.49%)`	⬇️
factory/network/networkComponents.go	`0.00% <0.00%> (ø)`
factory/network/networkComponentsHandler.go	`0.00% <0.00%> (ø)`
factory/status/statusComponents.go	`0.00% <0.00%> (ø)`
node/node.go	`66.19% <0.00%> (-0.19%)`	⬇️
outport/factory/outportFactory.go	`88.63% <50.00%> (+5.65%)`	⬆️
outport/host/driver.go	`81.81% <81.81%> (ø)`
outport/factory/hostDriverFactory.go	`89.47% <89.47%> (ø)`
api/groups/nodeGroup.go	`89.36% <100.00%> (+0.67%)`	⬆️
... and 2 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

iulianpascalau · 2023-05-12T06:36:12Z

cmd/node/config/external.toml

    # This flag shall only be used for observer nodes
    Enabled = false
-    URL = "localhost:22111"
+    # This flag will start the WebSocket connector as server or client


Maybe "ServerMode = false/true" ?

or Mode = "server"

changed in Mode

iulianpascalau · 2023-05-12T06:37:17Z

cmd/node/config/external.toml

-    URL = "localhost:22111"
+    # This flag will start the WebSocket connector as server or client
+    IsServer = false
+    # The url of the WebSocket client/server


I think we should clarify that this URL is used when using the driver in client mode.

This will be used in both cases because the URL is needed also for the server when it starts.

iulianpascalau · 2023-05-12T06:39:41Z

cmd/node/config/external.toml

    WithAcknowledge = true
    # Currently, only "json" is supported. In the future, "gogo protobuf" could also be supported
    MarshallerType = "json"
+    # The number of seconds when the client will try again to send the data
+    RetryDurationInSec = 5
+    BlockingAckOnError = false


add a descriptive comment on what this does?

iulianpascalau · 2023-05-12T06:40:40Z

outport/factory/hostDriverFactory.go

+
+var log = logger.GetOrCreate("outport/factory/hostdriver")
+
+func CreateHostDriver(args ArgsHostDriverFactory) (outport.Driver, error) {


missing comments in the whole file

iulianpascalau · 2023-05-12T06:42:36Z

outport/factory/hostDriverFactory_test.go

@@ -0,0 +1 @@
+package factory


missing unit tests

iulianpascalau · 2023-05-12T08:28:15Z

outport/host/driver.go

+	}, nil
+}
+
+func (o *hostDriver) SaveBlock(outportBlock *outport.OutportBlock) error {


missing comment

iulianpascalau · 2023-05-12T08:30:04Z

outport/host/driver.go

+
+	err = o.senderHost.Send(marshalledPayload, topic)
+	if err != nil {
+		o.log.Error("cannot send on route", "topic", topic, "error", err)


Since this still can happen while closing the node, I would filter out log.Error prints only if they are not of type closed error.

bogdan-rosianu · 2023-05-12T08:55:36Z

cmd/node/config/external.toml

    # This flag shall only be used for observer nodes
    Enabled = false
-    URL = "localhost:22111"
+    # This flag will start the WebSocket connector as server or client


or Mode = "server"

bogdan-rosianu · 2023-05-12T08:57:08Z

outport/host/driver.go

+	log        core.Logger
+}
+
+func NewHostDriver(args ArgsHostDriver) (*hostDriver, error) {


missing comment

iulianpascalau · 2023-05-12T12:23:00Z

cmd/node/config/external.toml

-    # This flag will start the WebSocket connector as server or client
-    IsServer = false
-    # The url of the WebSocket client/server
+    # This flag will start the WebSocket connector as server or client( can be "client" or "server")


typo:

as server or client (can be "client"

wrong spacing near the ( character

cmd/node/config/external.toml

outport/host/driver.go

gabi-vuls

System test passed
@@ Log scanner @@

integrate-new-ws-client-server

================================================================================

Known Warnings 10
New Warnings 3
Known Errors 0
New Errors 0
Panics 0
================================================================================
block hash does not match 6548
wrong nonce in block 2528
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0
================================================================================
No jailed nodes on the testnet
================================================================================

iulianpascalau

just the spacing comment

integrate the new web-sockets connector

0c39afd

miiu96 added 10 commits April 12, 2023 10:36

extra field in config

6ab26f4

small fix

1899b85

change core version

f774a29

latest core commit

71523ac

commit

162d334

new commit hash

2ebe548

integrate new host

7aa5793

move host driver

42f28c7

latest version of communication go

d42c502

update mx-chain-core and communication go

908ff49

miiu96 marked this pull request as ready for review May 11, 2023 07:28

miiu96 changed the title ~~New web-sockets connector [WIP]~~ New web-sockets connector May 11, 2023

iulianpascalau self-requested a review May 11, 2023 07:32

miiu96 added 3 commits May 11, 2023 10:57

fix

48fa297

change comm go

72ffd89

change comm go

4d5cc99

iulianpascalau reviewed May 12, 2023

View reviewed changes

miiu96 requested a review from bogdan-rosianu May 12, 2023 08:52

bogdan-rosianu reviewed May 12, 2023

View reviewed changes

fixes

ae1775f

bogdan-rosianu previously approved these changes May 12, 2023

View reviewed changes

iulianpascalau reviewed May 12, 2023

View reviewed changes

fix

fc3828f

miiu96 dismissed bogdan-rosianu’s stale review via fc3828f May 15, 2023 07:29

gabi-vuls previously approved these changes May 18, 2023

View reviewed changes

bogdan-rosianu previously approved these changes May 18, 2023

View reviewed changes

iulianpascalau reviewed May 18, 2023

View reviewed changes

fix comment

a58411d

miiu96 dismissed stale reviews from bogdan-rosianu and gabi-vuls via a58411d May 18, 2023 09:46

iulianpascalau approved these changes May 18, 2023

View reviewed changes

bogdan-rosianu approved these changes May 18, 2023

View reviewed changes

gabi-vuls approved these changes May 18, 2023

View reviewed changes

miiu96 merged commit 20bf440 into feat/outport-refactor May 18, 2023
4 checks passed

miiu96 deleted the integrate-new-ws-client-server branch May 18, 2023 10:11

schimih added the type:improvement label May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `web-sockets` connector #5170

New `web-sockets` connector #5170

miiu96 commented Apr 11, 2023 •

edited

codecov bot commented Apr 11, 2023 •

edited

iulianpascalau May 12, 2023

bogdan-rosianu May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 12, 2023

bogdan-rosianu May 12, 2023

bogdan-rosianu May 12, 2023

miiu96 May 12, 2023

iulianpascalau May 12, 2023

miiu96 May 18, 2023

gabi-vuls left a comment

iulianpascalau left a comment


		var log = logger.GetOrCreate("outport/factory/hostdriver")

		func CreateHostDriver(args ArgsHostDriverFactory) (outport.Driver, error) {

New web-sockets connector #5170

New web-sockets connector #5170

Conversation

miiu96 commented Apr 11, 2023 • edited

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Apr 11, 2023 • edited

Codecov Report

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

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

gabi-vuls left a comment

Choose a reason for hiding this comment

integrate-new-ws-client-server

iulianpascalau left a comment

Choose a reason for hiding this comment

New `web-sockets` connector #5170

New `web-sockets` connector #5170

miiu96 commented Apr 11, 2023 •

edited

codecov bot commented Apr 11, 2023 •

edited