Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(525)

Issue 4517125: Ns-3 BRITE Integration

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by Josh Pelkey
Modified:
9 years, 9 months ago
CC:
riley_ece.gatech.edu, ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

The Boston university Representative Internet Topology gEnerator (BRITE) is a topology generation framework built with flexibility and extensibility in mind. By integrating this topology framework with ns-3, users will have the power to quickly and efficiently create large Internet topologies, while taking advantage of ns-3's simulation capabilities. More documentation for the code is found in the source, as well as src/brite/doc/brite.rst

Patch Set 1 #

Total comments: 26
Unified diffs Side-by-side diffs Delta from patch set Stats (+1146 lines, -0 lines) Patch
M doc/main.h View 1 chunk +1 line, -0 lines 0 comments Download
M doc/models/Makefile View 1 chunk +1 line, -0 lines 0 comments Download
M doc/models/source/index.rst View 1 chunk +1 line, -0 lines 0 comments Download
A src/brite/doc/brite.rst View 1 chunk +136 lines, -0 lines 6 comments Download
A src/brite/examples/brite-generic-example.cc View 1 chunk +244 lines, -0 lines 6 comments Download
A src/brite/examples/conf_files/ASBarabasi.conf View 1 chunk +21 lines, -0 lines 1 comment Download
A src/brite/examples/conf_files/ASWaxman.conf View 1 chunk +23 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTBarabasi.conf View 1 chunk +22 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTBarabasi10.conf View 1 chunk +22 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTBarabasi20.conf View 1 chunk +22 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTBarabasi5.conf View 1 chunk +22 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTWaxman.conf View 1 chunk +25 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTWaxman10.conf View 1 chunk +25 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTWaxman20.conf View 1 chunk +25 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/RTWaxman5.conf View 1 chunk +25 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/TD_ASBarabasi_RTWaxman.conf View 1 chunk +50 lines, -0 lines 0 comments Download
A src/brite/examples/conf_files/seed_file View 1 chunk +6 lines, -0 lines 0 comments Download
A src/brite/examples/wscript View 1 chunk +5 lines, -0 lines 1 comment Download
A src/brite/helper/brite-topology-helper.h View 1 chunk +146 lines, -0 lines 8 comments Download
A src/brite/helper/brite-topology-helper.cc View 1 chunk +219 lines, -0 lines 4 comments Download
A src/brite/test/examples-to-run.py View 1 chunk +20 lines, -0 lines 0 comments Download
A src/brite/waf View 1 chunk +2 lines, -0 lines 0 comments Download
A src/brite/wscript View 1 chunk +78 lines, -0 lines 0 comments Download
M src/wscript View 3 chunks +3 lines, -0 lines 0 comments Download
M test.py View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 2
Tommaso Pecorella
Nice work. I have some comments, mostly in the file. As a general comment I've ...
9 years, 9 months ago (2011-06-02 14:30:14 UTC) #1
Tom Henderson
9 years, 9 months ago (2011-06-07 18:24:53 UTC) #2
A few additional comments:
- there is no test suite, so I would not +1 to merge until this is remedied

- BRITE has router-level and AS-level topology generators, which you are turning
to router-level topologies.  I am not sure about just dumping in the AS*.conf
files as examples without mentioning this distinction somewhere in the
documentation (i.e. that ns-3 doesn't support AS-level topology building yet).

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst
File src/brite/doc/brite.rst (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode42
src/brite/doc/brite.rst:42: An example, brite-generic-example, is provided to
show the user how to do this.
It is not really clear from the above description which type of ns-3 links are
created, and that if there are brite attributes such as delay and bandwidth,
then these attributes will be set accordingly on the ns-3 links.

You have more details on this aspect embedded in comments in your example; this
should be highlighted more here in the documentation.

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode46
src/brite/doc/brite.rst:46: in case you wish to randomize subsequent runs.
Finally, it saves the most
Why not just dynamically create a temporary seed_file from an ns-3
RandomVariable before invoking brite?

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode131
src/brite/doc/brite.rst:131: nix:         Enables nix-vector routing. Global
routing is used by default.
toggling this option and running against a set of topologies could be a nice way
to profile the run-time performance of nix-vector vs global-routing

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
File src/brite/examples/brite-generic-example.cc (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:81: /*
suggest a command line option rather than commented out block.  Suggest to
default to on, since you tell people in the manual about the --vis option but
not that by default the placement is random.

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:145: // assigned are going to be all
over the place on a specfic node's
specific

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:147: // which IPs correspond to
which node's netdevices later on.
I think it wouldn't be that hard to do a better job on this aspect, by assigning
addresses to links according to subnet boundaries.  Either perform address
assignment as you go about defining the links, or else extend the helper to do
this magically for the user.  For instance, you could have a method that allowed
you to assign prefixes intelligently to all links, and choose the mask size
(default /30).

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:171: Address remoteAddress
(InetSocketAddress (ipv4Sink, 1024));
these types of code blocks in our examples (lines 168-171) cry out for more
helper functionality (perhaps using the Object name system)

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:236: /*
suggest to add as command-line options to enable this output. Agree with Tommaso
that perhaps it may be better to wrap this with documented helper methods.

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/conf_files/AS...
File src/brite/examples/conf_files/ASBarabasi.conf (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/conf_files/AS...
src/brite/examples/conf_files/ASBarabasi.conf:2: 
I would recommend that if you keep these conf files in ns-3 (copied over from
brite library), then you should prepend with a copyright/license statement from
brite.

Another option would be to avoid copying these over and to just ask the user to
open a conf file from the brite library itself.

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
File src/brite/helper/brite-topology-helper.h (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:32: 
\defgroup brite

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:34: * \brief Interface with BRITE, the
Boston university Representative Internet
\ingroup brite

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:132: brite::Topology* GetBriteTopology
(void);
On 2011/06/02 14:30:14, Tommaso Pecorella wrote:
> The above function is "dangerous", as it returns the pointer to a private
> variable (not used after the initialization, tho).
> My question is: is this really necessary? Check the comments in the example as
> well.

If deemed necessary/useful to keep this, it should be clear in the documentation
what the ownership is of the raw pointer being passed.  We have a few instances
of these raw pointers in our public APIs but they should have suitable
documentation around them.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b