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
Use setTimer on Network.discover #919
Conversation
|
687d753
to
b498e04
Compare
86aeb98
to
0dfc8e0
Compare
I applied @AndrejMitrovic suggestions. |
source/agora/node/FullNode.d
Outdated
this.taskman.wait(5.seconds); | ||
} | ||
}); | ||
scope void discover () { this.network.discover(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. It should not be a scope
delegate. If it's scope, it will be allocated on the stack. So when start()
returns and the timer tries to call discover
, it may crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I was worried.
source/agora/node/Validator.d
Outdated
this.network.discover(required_peer_keys); | ||
this.taskman.wait(5.seconds); | ||
} | ||
scope void discover () { this.network.discover(required_peer_keys); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also make it non-scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's removed.
source/agora/node/FullNode.d
Outdated
this.taskman.wait(5.seconds); | ||
} | ||
}); | ||
void discover () { this.network.discover(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the startPeriodicDiscovery function anymore. And also:
Sorry I thought that we don't need the runTask
, but actually it would make sense to run it inside a task to make it non blocking.
How about we use the same technique which you did in the Validator?
So startPeriodicDiscovery
with a runTask
inside, and start would call it first and then startPeriodicCatchup
. Inside startPeriodicDiscovery
you would have this void discover
and the setting up of the timer.
Do you see what I mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to make the FullNode and Validator consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to be consistent. It's been applied.
source/agora/node/FullNode.d
Outdated
@@ -154,7 +154,7 @@ public class FullNode : API | |||
|
|||
public void start () | |||
{ | |||
this.startPeriodicDiscovery(); | |||
startPeriodicDiscovery(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just missing this.
and then it's ready to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch is too fast!
These need to be replaced to use setTimer once TaskManager is fixed to use it.
Codecov Report
@@ Coverage Diff @@
## v0.x.x #919 +/- ##
==========================================
+ Coverage 90.24% 90.42% +0.17%
==========================================
Files 71 71
Lines 5650 5669 +19
==========================================
+ Hits 5099 5126 +27
+ Misses 551 543 -8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Two improvements are needed here.
Taskmanager.setTimer
iswait
andcall
action.So I call first to eliminate the delay in the first run.
Currently, this kind of movement causes the error as below.
Second
Validator.startPeriodicDiscovery
This is unnecessary repetitive behavior of
buildRequiredKeys
Relates #872