benchmark: fix pht insert test #184
Conversation
- Old log code updated; - fix output of entries under final prefix locations;
|
I also should mention that the test will fail to run properly due to the bug from #180. I have made sure the test works well on my end by reverting the SockAddr changes from the python layer. |
| string toString() const | ||
| string flagsToString() const | ||
| size_t size_ |
There was a problem hiding this comment.
IMHO fields should not be exposed here. Even in the C++ code, these fields are only public for convenience but should be made private, since it's dangerous to manipulate them directly (risk of inconsistence, like size_ larger than content_ size()*8 or flags_.size()*8).
edit: this refers to lines 194-196
There was a problem hiding this comment.
Totally agreed with that, since flags_ and content_ are internal variable, user shouldn't be able to edit them directly.
As @aberaud point out, there is a risk of inconsistence ( ou just mistake if you [try to] move around those bits )
| PhtTest.prefix = prefix.decode() | ||
| DhtNetwork.log('Index name: <todo>') | ||
| DhtNetwork.log('Leaf prefix:', prefix) | ||
| PhtTest.prefix = str(prefix) |
There was a problem hiding this comment.
Since the goal of this patch is to "fix" the pht test, please also make it use non-static callbacks and fields so we can run multiple instances at the same time. Current design is awful.
| @@ -471,6 +473,25 @@ cdef class DhtRunner(_WithID): | |||
| ref.Py_DECREF(<object>token._cb['cb']) | |||
| # fixme: not thread safe | |||
|
|
|||
| cdef class Prefix(object): | |||
| cdef shared_ptr[cpp.Prefix] _prefix | |||
There was a problem hiding this comment.
Why not just cpp.Prefix (like for InfoHash and others) ? __cinit__can be used for initialization. The API doesn't make use of shared_ptr<Prefix>
|
|
||
| @staticmethod | ||
| def lookupDoneCb(ok): | ||
| DhtNetwork.log('[LOOKUP]:', PhtTest.key, "--", "success!" if ok else "Fail...") | ||
| DhtNetwork.Log.log('[LOOKUP]:', PhtTest.key, "--", "success!" if ok else "Fail...") |
There was a problem hiding this comment.
Log.log, looks like "a lot" of "Log"
There was a problem hiding this comment.
@kaldoran: What do you mean precisely? Dhtnetwork.Log is a class which has methods log, warn and err.
Do you suggest changing Log.log method to Log.debug?
There was a problem hiding this comment.
Indeed, in order to avoid future mistake. [Log.debug or some other form]
| string toString() const | ||
| string flagsToString() const | ||
| size_t size_ |
There was a problem hiding this comment.
Totally agreed with that, since flags_ and content_ are internal variable, user shouldn't be able to edit them directly.
As @aberaud point out, there is a risk of inconsistence ( ou just mistake if you [try to] move around those bits )
aberaud
force-pushed
the
master
branch
2 times, most recently
from
0733bb6
to
0abd288
Compare
aberaud
force-pushed
the
master
branch
2 times, most recently
from
0d272df
to
8b6dcb1
Compare
|
I realize that this PR has been opened for a very long time and I have not found time to work on this again yet. Since I cannot say as of now when I will find the time to work on this again, I think that the best is to close this for consistency of the list of ongoing PRs. Let anyone be free to take this work and produce a mergeable version of it. |
|
@sim590 Thanks I'll follow up on this PR |
aberaud
force-pushed
the
master
branch
9 times, most recently
from
f05b60d
to
dfd0a09
Compare
aberaud
force-pushed
the
master
branch
14 times, most recently
from
966e620
to
fe6676f
Compare
Output from
insertTestin benchmark has been wrong since the begining due to minor errors. This PR fixes those errors and also depends on #183 to build and run right. Keep that in mind before merging.I have attached
before.txtandafter.txtfiles to compare outputs.before.txt
after.txt