Add new turtle test node based directly on NodeBase #353
Conversation
gavin-norman-sociomantic
requested review from
nemanja-boric-sociomantic and
mihails-strasuns
|
I believe the separation of turtle node from the actual node implementation is a historical artefact from the days when the fake nodes lived partially in turtle. It seems to be unnecessary now. |
|
sociomantic-tsunami/dlsproto#101 gives a rough example of how this change simplifies the code for a fake node. |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
96304fc to
4869547
Compare
|
Hm, this doesn't look like any code I have written and turtle definitely doesn't care so I'd say "whatever fits you best" :) |
|
It was derived from code in turtle that you wrote originally. |
|
I pretty sure that it was you who wrote that code in turtle originally :) I copied it to swarm with almost no changes when splitting things. |
src/turtle/env/model/TestNode.d
Outdated
| Notes: | ||
| 1. Restarting the node *does not* clear any data in its storage | ||
| engine. To do that, call clear(). | ||
| 2. You must call stop() first, before calling restart(). |
There was a problem hiding this comment.
This is extremely weird tbh. If you need to call stop first, it is just start, not restart.
There was a problem hiding this comment.
Agreed. I have no memory of why we did it like this.
Could be :) Mainly I'm interested in your opinion on the concept, and whether I'm missing some subtlety of turtle. |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
4869547 to
806a972
Compare
|
AFAIR turtle only cares that nodes stop printing messages once |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
806a972 to
91403ab
Compare
|
Adapted with working restart. (Still super rough.) |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
91403ab to
c159506
Compare
|
Tidied up. |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
c159506 to
9e3915a
Compare
|
D2 fixes. |
|
Still TODO:
|
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
9e3915a to
e1d4868
Compare
gavin-norman-sociomantic
changed the title
src/turtle/env/model/TestNode.d
Outdated
|
|
||
| static this ( ) | ||
| { | ||
| log = Log.lookup("testnode"); |
There was a problem hiding this comment.
Hmmm. Is there a way for the fake node implementation to override the testnode log name? It's useful when testing with multiple types of test nodes.
There was a problem hiding this comment.
Yeah, I'll change it to one logger per instance.
| ConnectionSetupParams conn_setup_params, Options options, int backlog ) | ||
| { | ||
| verify(!already_created, "Can only have one " ~ idup(this.id) ~ | ||
| " per turtle test app"); |
There was a problem hiding this comment.
This is a case for assert?
|
|
||
| this.register(theScheduler.epoll); | ||
| if ( Task.getThis() ) | ||
| theScheduler.processEvents(); |
There was a problem hiding this comment.
Is there a case where this is not running in task? With the turtle, I don't think so, but I might be mistaken.
There was a problem hiding this comment.
(note that you require that theScheduler.epoll is something - perhaps that's worth verifying as well?)
src/turtle/env/model/TestNode.d
Outdated
|
|
||
| ***************************************************************************/ | ||
|
|
||
| abstract public void clear ( ); |
There was a problem hiding this comment.
Hm, what's the reason for having this abstract method?
There was a problem hiding this comment.
True, it's not required at this level.
| ( Client.Neo.Get.Notification, Const!(Client.Neo.Get.Args) ) { }); | ||
| if ( !ok ) | ||
| return false; | ||
|
|
There was a problem hiding this comment.
if (value[] != "hello") return false;
|
Needs rebase now. |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
e1d4868 to
2563071
Compare
|
Rebased. |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
2563071 to
49137d7
Compare
|
Hmmmm |
|
I also left a review on this PR :-) |
|
Ohh shit. I changed |
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
49137d7 to
b3c7934
Compare
References to the listeners are required by INodeBase, but these were previously passed into the ctor from outside, rather than being owned and controlled by this class. This, combined with the lack of support in ocean for reopening a shutdown listener, made it impossible to implement a method to restart the node's listeners. (The ideal solution, of course, would be to add the missing restart support to the select listener in ocean, but this will require a major swarm release.)
(As a counterpart to the existing stopListener.)
There's more than one listener now.
This is intended as a replacement of the existing turtle.env.model.Node. The reason for the new class is that the turtle/swarm node framework is currently fairly complicated, with several different classes required to implement a fake node. The new TestNode derives directly from NodeBase (the standard base class for swarm nodes), whereas the older Node does not. This means that to implement a fake node using TestNode, one does not have to separately write a class deriving from NodeBase.
gavin-norman-sociomantic
force-pushed
the
nodeenv
branch
from
b3c7934 to
62c2478
Compare
|
Updated. |
|
@gavin-norman-sociomantic was there anything left for this PR from your side? It looks fine to me but there is a plenty of neo stuff I have no idea about :) |
|
The PR looks sensible to me, in isolation. I think it was waiting on trying to adapt one of the proto fake nodes, to confirm that it works (and is helpful) in practice. |
|
Ah, this appears to be a test of adapting a *proto to use this: sociomantic-tsunami/dlsproto#101 |
(Currently based on #350. Only the last commit is relevant.)