Task #2702
closed
ping: refactor for separate responsibility
Added by Junxiao Shi about 9 years ago.
Updated about 9 years ago.
Description
Refactor ndnping program to use the following structure:
|------| -> Tracer
Face <- | |
| Ping | -> StatisticsCollector
Options -> | | |
| | v
|------| -> printStatistics
- An
Options POD struct contains parameters parsed from command line.
Ping class constructor should take Face& and Options as argument; getter/setter for each option are deleted from the class.Ping class shouldn't write to stdout directly; instead, it emits a signal for each successful/failed ping, and when all pings are completed.- A
Tracer class connects to ping succeed/fail signals, and writes logs for each successful/failed ping.
- A
StatisticsCollector class connects to ping succeed/fail signals, and collects statistics.
- A
printStatistics function connects to completion signal, and prints statistics.
Benefits:
- separate responsibility:
Ping performs network operations, Tracer writes logs, StatisticsCollector collects statistics.
- unit testing: passing DummyClientFace to
Ping constructor allows unit testing.
- simulation:
Ping and StatisticsCollector can be reused in a ndnSIM scenario.
+1
I would also like convert to use Boost.ProgramOptions library instead of getopt.
- Status changed from New to In Progress
- Assignee set to Eric Newberry
This is a complex Task.
It's recommended that Assignee should first design headers and submit for review, before writing implementation code.
Here's what I was thinking for the header file. I used the Boost::Signals2 library for signals.
- Code reviews should be uploaded to Gerrit.
- Have one .hpp (and .cpp) per component, not a big .hpp.
- Use
ndn::util::signal instead of Boost.Signals2. As benchmarked in #2116 note-35, Boost.Signals2 is too slow.
Got it. Should I upload what I have to Gerrit, even if it's not finished?
Got it. Should I upload what I have to Gerrit, even if it's not finished?
Whenever you need feedback for some code, upload to Gerrit, even if it's just headers.
You may mark CodeReview-2 to prevent merging.
- File deleted (
ndn-ping.hpp)
- % Done changed from 0 to 90
Everything is currently implemented on Gerrit with the exception of Boost.ProgramOptions, which I'm working on right now.
I believe I misunderstood what the role of Face was. Should output to stdout be done through Tracer?
Should output to stdout be done through Tracer?
This depends on what is being written:
- Banner, usage, version number, etc: written by the main function, or a helper function called by the main function.
- Log line for each successful or failed probe: written by
Tracer.
- Statistics at end of the session: written by
printStatistics function.
If anything, we should use uint32_t for m_nextPingNo, as that's what Interest::setNonce() expects as input.
No. ping reference number is not Nonce.
Ping uses startPingNo to determine whether to randomly generate the ping number (when set to -1, which is signed), so we have two options:
- Keep it as
int64_t/int32_t
- Add another boolean value to options called shouldGenerateRandomPingNo and then use
uint64_t/uint32_t
Which should I do?
It's better to be more explicit in Options struct.
/** \brief start sequence number
*/
uint32_t startSeq;
/** \brief whether to generate random start sequence number
*
* If true, a random start sequence number is generated,
* and \p startSeq is ignored.
*/
bool wantRandomStartSeq;
In commit:cb4818c6ca423f11ae1e11c7daa5725c66d15dba, I notice that RTTs are printed as integers, but operators need to see higher precision.
Suggestion:
- in
ping.hpp, declare typedef time::duration<double, time::milliseconds::period> Rtt;
- change second parameter of
Ping::afterResponse signal to Rtt
- don't use division in
StatisticsCollector::recordResponse
- don't use
duration_cast in Tracer::onResponse
- Status changed from In Progress to Closed
- % Done changed from 90 to 100
Code has been reviewed and merged.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by