Rework NetworkManager using tasks #889

AndrejMitrovic · 2020-05-29T05:03:06Z

Works for me locally, let's see what the CI says.

There might be some missing scope / @safe and so on, I'll add those if neccessary.

Fixes #587

codecov · 2020-05-29T05:11:59Z

Codecov Report

Merging #889 into v0.x.x will increase coverage by 0.06%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           v0.x.x     #889      +/-   ##
==========================================
+ Coverage   90.34%   90.41%   +0.06%     
==========================================
  Files          70       70              
  Lines        5643     5652       +9     
==========================================
+ Hits         5098     5110      +12     
+ Misses        545      542       -3

Flag	Coverage Δ
#integration	`54.77% <85.57%> (-0.14%)`	⬇️
#unittests	`88.76% <87.50%> (+0.08%)`	⬆️

Impacted Files	Coverage Δ
source/agora/network/NetworkManager.d	`78.28% <86.81%> (+2.84%)`	⬆️
source/agora/network/NetworkClient.d	`88.88% <100.00%> (ø)`
source/agora/node/FullNode.d	`84.84% <100.00%> (+0.47%)`	⬆️
source/agora/node/Validator.d	`75.00% <100.00%> (+2.58%)`	⬆️
source/agora/test/Base.d	`87.75% <0.00%> (-0.41%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09bcfe5...f7a0fc5. Read the comment docs.

AndrejMitrovic · 2020-05-29T06:20:10Z

Oh and taskManager.schedule will be very useful to have once we have that. It should make it more efficient. But it's not a strict requirement.

Geod24

That's a big diff

Geod24 · 2020-05-29T05:17:37Z

source/agora/common/Config.d

+    /// The maximum number of connection tasks to use during network discovery
+    public size_t connection_tasks = 10;
+


It's a bit eerie to expose this in a config file that might be touched by not-so-technical ppl.

Should we just make it a constant then? I'm not sure which number to use. But I guess we can start with a low number like 10 and then increase it as we see fit?

Geod24 · 2020-05-29T05:18:39Z

source/agora/node/FullNode.d

+                scope (success)
+                    this.taskman.wait(5.seconds);


Why the scope (success) ?

Because I don't want to suspend the fiber if an exception is thrown. I want it to kill the fiber instead.

And now that I said that out loud, I realize I should probably make discover nothrow.

Buuuuuuuttttttttt... Why not simply:

while (true) { this.network.discover(); this.taskman.wait(5.seconds); }

The wait is in a scope (success) in a loop with a single statement. Either the statement throws and the code is never executed, or the statement doesn't throw and the code is executed. What am I missing ?

Ahh right, my bad. It was copied over from the other example usage where I have a continue. I'll fix it.

source/agora/node/Validator.d

Geod24 · 2020-05-29T05:39:21Z

source/agora/network/NetworkManager.d

@@ -57,31 +57,288 @@ mixin AddLogger!();
 /// Ditto
 public class NetworkManager
 {
+    /// Node information
+    private static struct Node


Should this be something less ambiguous, like NodeInfo ?

I had that exact same thought. But then I realized we already have that struct, returned in getNodeInfo. I need to think of another name..

Geod24 · 2020-05-29T05:43:28Z

source/agora/network/NetworkManager.d

+                    key = client.getPublicKey();
+                    break;
+                }
+                catch (HTTPStatusException ex)  // vibe.d (non-unittests)


What happens in LocalRest ?

it returns PublicKey.init. This was implemented this way as a workaround when splitting up full node vs validator node, Henry's PR.

Geod24 · 2020-05-29T05:44:19Z

source/agora/network/NetworkManager.d

+
+        ***********************************************************************/
+
+        public this (void delegate (Set!Address addresses) onNewAddresses )


Suggested change

public this (void delegate (Set!Address addresses) onNewAddresses )

public this (void delegate (Set!Address addresses) onNewAddresses)

Geod24 · 2020-05-29T06:19:52Z

source/agora/network/NetworkManager.d

+                scope (success)
+                    this.outer.taskman.wait(1.seconds);


Ditto why scope success ?

In this specific case it's because of the continue below. I didn't want to duplicate the wait call.

AndrejMitrovic · 2020-05-29T06:37:25Z

Fixed styling
Added comment about the special-case in LocalRest
scope (success) was used because if the Fiber shuts down, the sleep call is unnecessary
Renamed Node => NodeConnInfo (didn't call it NodeInfo to avoid clashes with the existing struct).
Removed the config option.
Sorry for the big diff.

Geod24

Nice. Definitely the right directions. I'm not sure I can give this the review it deserves in the time remaining in the sprint, but I really, really want to get this through the door.

Typo in the commit message: "does not [missing 'need' I guess] to be re-triggered"

source/agora/network/NetworkManager.d

Geod24 · 2020-05-29T08:45:25Z

source/agora/node/FullNode.d

+                scope (success)
+                    this.taskman.wait(5.seconds);


Buuuuuuuttttttttt... Why not simply:

while (true) { this.network.discover(); this.taskman.wait(5.seconds); }

The wait is in a scope (success) in a loop with a single statement. Either the statement throws and the code is never executed, or the statement doesn't throw and the code is executed. What am I missing ?

Geod24 · 2020-05-29T08:47:52Z

source/agora/network/NetworkManager.d

+                    if (this.outer.banman.isBanned(this.address))
+                    {
+                        this.onFailedConnection(this.address);
+                        return;
+                    }


My inner developer suggests me that we might want to make onFailedConnection returns a boolean telling whether we should retry or abort the request, and that we could move the isBanned check in there.
I don't know how sensible this would be, and I'm not asking you to do it, just raising possibilities.

Oh this is a good idea.

AndrejMitrovic · 2020-05-29T08:54:50Z

Nice. Definitely the right directions. I'm not sure I can give this the review it deserves in the time remaining in the sprint, but I really, really want to get this through the door.

If there's anything missing (I'm sure there is), we can continue work on it. Filing another issue with the rest of the problems that have to be fixed.

AndrejMitrovic · 2020-05-29T09:23:41Z

There are random failures due to #887 and also the occasional failure by dub not finding a file. But otherwise it's ready to go.

source/agora/network/NetworkManager.d

AndrejMitrovic · 2020-05-30T14:49:36Z

Fixed wrong docs.

Geod24 · 2020-05-31T05:49:06Z

Full error: core.exception.AssertError@source/agora/consensus/protocol/Nominator.d(372): Assertion failure

This triggers quite a bit...

It was the only API endpoint wrapper which stored the key to an internal public field, but this is unnecessary as the cached key is only used in one scope.

Grouping them together with the other sets.

Using the ban manager with an infinite ban time is awkward, and further complicates the situation where the node's IP changes. The Ban data is persistent and this is problematic if the Node's IP changes but we still have an IP in the ban list that is no longer used.

The new NetworkManager now uses separate connection and address discovery tasks. The 'connection_tasks' config option has been added to fine-tune the number of connection tasks a Node should use. The discover() method can now be called multiple times, which is required to be able to support quorum reshuffling. The address discovery is now an endless task which does not need to be re-triggered, it runs in the background and queries each client in turn every 5 seconds. The periodic catchup routine no longer waits for full node discovery before attempting to read blocks. It is unnecessary to have to wait for minPeersConnected() to be true, catchup can proceed as soon as we have at least one node connection. Fixes bosagora#587

Geod24 · 2020-05-31T19:15:02Z

Rebased on v0.x.x

AndrejMitrovic added the type-enhancement An improvement of existing functionalities label May 29, 2020

AndrejMitrovic force-pushed the new-netman branch from acf6375 to ad8f906 Compare May 29, 2020 05:03

AndrejMitrovic changed the title ~~Rework NetworkManager~~ Rework NetworkManager using tasks May 29, 2020

Geod24 reviewed May 29, 2020

View reviewed changes

AndrejMitrovic force-pushed the new-netman branch from ad8f906 to cb94ff1 Compare May 29, 2020 06:34

AndrejMitrovic force-pushed the new-netman branch from cb94ff1 to 9939aa2 Compare May 29, 2020 06:39

AndrejMitrovic mentioned this pull request May 29, 2020

Implement shell quorum balancing #845

Merged

Geod24 reviewed May 29, 2020

View reviewed changes

AndrejMitrovic force-pushed the new-netman branch 2 times, most recently from e3e2eda to d0ade1d Compare May 29, 2020 09:07

Geod24 reviewed May 29, 2020

View reviewed changes

source/agora/network/NetworkManager.d Outdated Show resolved Hide resolved

AndrejMitrovic force-pushed the new-netman branch from d0ade1d to 4207610 Compare May 30, 2020 14:49

Geod24 approved these changes May 31, 2020

View reviewed changes

AndrejMitrovic added 4 commits June 1, 2020 04:14

NetworkClient: Make getPublicKey() consistent

70ca7a0

It was the only API endpoint wrapper which stored the key to an internal public field, but this is unnecessary as the cached key is only used in one scope.

Move required_peer_keys to the right place

d41b97a

Grouping them together with the other sets.

Geod24 force-pushed the new-netman branch from 4207610 to f7a0fc5 Compare May 31, 2020 19:14

Geod24 merged commit f8607d0 into bosagora:v0.x.x May 31, 2020

AndrejMitrovic deleted the new-netman branch May 31, 2020 23:04

AndrejMitrovic mentioned this pull request Jun 15, 2020

Design and implement quorum balancing #240

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework NetworkManager using tasks #889

Rework NetworkManager using tasks #889

AndrejMitrovic commented May 29, 2020

codecov bot commented May 29, 2020 •

edited

Loading

AndrejMitrovic commented May 29, 2020

Geod24 left a comment

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

Geod24 May 29, 2020

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

AndrejMitrovic commented May 29, 2020 •

edited

Loading

Geod24 left a comment

Geod24 May 29, 2020

Geod24 May 29, 2020

AndrejMitrovic May 29, 2020

AndrejMitrovic commented May 29, 2020

AndrejMitrovic commented May 29, 2020

AndrejMitrovic commented May 30, 2020

Geod24 commented May 31, 2020

Geod24 commented May 31, 2020

		/// The maximum number of connection tasks to use during network discovery
		public size_t connection_tasks = 10;


		***********************************************************************/

		public this (void delegate (Set!Address addresses) onNewAddresses )

Rework NetworkManager using tasks #889

Rework NetworkManager using tasks #889

Conversation

AndrejMitrovic commented May 29, 2020

codecov bot commented May 29, 2020 • edited Loading

Codecov Report

AndrejMitrovic commented May 29, 2020

Geod24 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic commented May 29, 2020 • edited Loading

Geod24 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

AndrejMitrovic commented May 29, 2020

AndrejMitrovic commented May 29, 2020

AndrejMitrovic commented May 30, 2020

Geod24 commented May 31, 2020

Geod24 commented May 31, 2020

codecov bot commented May 29, 2020 •

edited

Loading

AndrejMitrovic commented May 29, 2020 •

edited

Loading