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. |
| Prefix(Prefix& p) except + | ||
| 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.
| # 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>
kaldoran left a comment
kaldoran left a comment There was a problem hiding this comment.
As aberaud point out, there is some changes to do before accepting this PR. :)
| @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]
| Prefix(Prefix& p) except + | ||
| 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 aberaud force-pushed the master branch 4 times, most recently from 22c89fb to 122c30c Compare 3 participants
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