Issue 204055: Topology read system

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+11812 lines, -0 lines)			Patch
A	examples/topology-read/Inet_small_toposample.txt	View	3	1 chunk	+20 lines, -0 lines	0 comments	Download
A	examples/topology-read/Inet_toposample.txt	View	3	1 chunk	+7826 lines, -0 lines	0 comments	Download
A	examples/topology-read/Orbis_toposample.txt	View	3	1 chunk	+2769 lines, -0 lines	0 comments	Download
A	examples/topology-read/topology-example-sim.cc	View	1 3 4 5 6 7 8 9	1 chunk	+209 lines, -0 lines	0 comments	Download
A	examples/topology-read/waf	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	examples/topology-read/wscript	View		1 chunk	+5 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/inet-topology-reader.h	View	1 2 3 4 5 6 7 8	1 chunk	+76 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/inet-topology-reader.cc	View	1 2 3 4 5 6 7 8	1 chunk	+140 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/orbis-topology-reader.h	View	1 2 3 4 5 6 7 8	1 chunk	+73 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/orbis-topology-reader.cc	View	1 2 3 4 5 6 7 8	1 chunk	+121 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/topology.h	View		1 chunk	+32 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/topology-reader.h	View	5 6 7 8	1 chunk	+203 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/topology-reader.cc	View	5 6 7 8	1 chunk	+169 lines, -0 lines	0 comments	Download
A	src/contrib/topology-read/wscript	View	1 4	1 chunk	+17 lines, -0 lines	0 comments	Download
A	src/helper/topology-reader-helper.h	View		1 chunk	+66 lines, -0 lines	0 comments	Download
A	src/helper/topology-reader-helper.cc	View		1 chunk	+80 lines, -0 lines	0 comments	Download
M	src/helper/wscript	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	src/wscript	View	9	1 chunk	+1 line, -0 lines	0 comments	Download
M	test.py	View	3	1 chunk	+2 lines, -0 lines	0 comments	Download

http://codereview.appspot.com/204055/diff/1/15 File src/contrib/topology-read/topology-reader-interface.h (right): http://codereview.appspot.com/204055/diff/1/15#newcode32 src/contrib/topology-read/topology-reader-interface.h:32: using namespace std; On 2010/02/08 06:48:49, Pavel Boyko wrote: ...

15 years, 2 months ago (2010-02-09 01:54:18 UTC) #3

Tommaso Pecorella

Great. Another file uploaded by mistake due to ultra-zelant check-style.py. We should use it on ...

15 years, 2 months ago (2010-02-09 01:58:41 UTC) #5

tpecorella_mac.com

Yo all, I need a +1 on this patch, or more comments to fix it ...

15 years, 2 months ago (2010-02-20 11:45:41 UTC) #6

Tommaso Pecorella

Yo all, I need a +1 on this patch, or more comments to fix it ...

15 years, 2 months ago (2010-02-20 14:42:18 UTC) #7

Pavel Boyko

Well, the main remaining issue is to convert examples to tests. I will not vote ...

15 years, 2 months ago (2010-02-21 10:33:53 UTC) #8

Tommaso Pecorella

Thanks for your reply. I was indeed waiting for feedbacks about the two parts (module ...

15 years, 2 months ago (2010-02-21 14:35:14 UTC) #9

Tommaso Pecorella

http://codereview.appspot.com/204055/diff/4008/4014 File examples/topology-read/topology-example-sim.cc (right): http://codereview.appspot.com/204055/diff/4008/4014#newcode106 examples/topology-read/topology-example-sim.cc:106: std::cout << "trying to read " << input << ...

15 years, 2 months ago (2010-02-22 00:12:04 UTC) #11

Mathieu Lacage

I did not go through the details but my main suggestions would be something along ...

15 years, 2 months ago (2010-02-26 15:16:51 UTC) #13

Josh Pelkey

Hi Tommaso, George Riley and I just looked over this. Overall we are happy with ...

15 years, 2 months ago (2010-02-26 19:42:29 UTC) #14

Tommaso Pecorella

Thanks Mathieu, Josh and George for the valuable comments. I'll start with Mathieu's points. On ...

15 years, 2 months ago (2010-02-27 03:08:51 UTC) #15

Thanks Mathieu, Josh and George for the valuable comments.

I'll start with Mathieu's points.

On points 1, 3 I have no problems.

Point 2 is more tricky.
> 2) change TopologyLink to contain Ptr<Node> instead of a node name: this will
> save the user from having to manage the std::map<std::string, TopologyNode >

Your proposal is indeed logical, but it does contain hidden nightmares. Let me
explain.
You need a list of the nodes and their attributes, and you need also to quick
access to them with an index. The most logical index would be the node's
pointer, but you can't made a map with the node pointer as first index, as it
would be a mess due to how the map works. So the node "name" is the logical
index.

The best thing is to add the node pointers to the TopologyLink, so the user
doesn't really need to go trough the map. However the map is stil needed,
otherwise accessing the node attributes will require a search trough a vector or
a list, which is not really user-friendy. I know that at the moment node
attributes aren't used at all, but they're there to hold AS annotations, which
is a really important feature. Anyway, now the TopologyLink now holds both the
node name and the node pointer. Redundant, but it does the trick.

The last point I would need to fully streamline the interface and make the map
opaque to the user would be to declare it an optional (default null) parameter.
However I'm stuck at that. How can I set a default value for a reference to a
map, as is a reference to a template? It might seems rather trivial, but I have
no idea about how to do it. I don't even know if it's possible at all.
Another other option would be to have two functions, so I added a "ReadSimple",
which doesn't need any map. I could also streamline it a bit more by defining
also a TopologyLinkSimple without the node names, but I think it's really
unnecessary.

Point 4 leaves me a bit puzzled. We can define some iterators, but the
TopologyLink and TopologyNode structures are "owned" by the calling code. The
readers just have references to them, but it doesn't hold them in private
variables. To have iterators I should move those structures to the reader. It
can be done of course, but it would require some work. I'll think about it
tomorrow before posting the new version. However I feel that doing so we'll hide
too much stuff to the calling function.
By the way, while a TopologyLinkIterator is useful, as you'll scan all the link
sequentially to build up the network, a NodeIterator isn't useful for real, as
it's a map. The "normal" operation sequence (when we'll have AS annotations ofc)
is:
- pick a link
- make the node interfaces
- make the link itself
- assing IP numbers based on the nodes annotations
For the last point you don't need an iterator. That's why I used a map. It seems
a bit weird at first, but it isn't. AS annotation is really a property of the
node, not the link, so to decide the IP number you have to check both link
endpoints and see if both belongs to the same AS or not. Hence, why the users
should iterate over that map ?


Now to Josh and George points.
1)
// #define NS3_LOG_ENABLE // Now defined by Makefile -- removed

2)
examples/topology-read/topology-example-sim.cc:89: Ptr<TopologyReaderInterface>
inFile = 0;
Not necessary for merge, but maybe a TopologyReaderHelper could take care of
this section?

Yes, it could. I avoided to define an helper on purpose, as the code is located
in the contrib directory.

3)
what happens when gsl is not available or not enabled?

The example will not compile of course. Good point. I'll clean out the example,
it's not really needed to use gsl there. A simple print of some values is more
than enough. Moreover the example just reads a 10-links file, so there's no need
to use gsl there to have a fancy histogram.

Tomorrow (it's 4am here) I'll fix the example code and I'll post the new
revision.

Best regards,

Tommaso

Josh Pelkey

Considering your latest patch and your comments on our last remaining issue about TopologyLink, +1 ...

15 years, 2 months ago (2010-02-27 19:56:45 UTC) #17

Mathieu Lacage

On Sat, Feb 27, 2010 at 4:08 AM, <tommypec@gmail.com> wrote: >> 2) change TopologyLink to ...

15 years, 2 months ago (2010-03-01 09:38:52 UTC) #18

On Sat, Feb 27, 2010 at 4:08 AM,  <tommypec@gmail.com> wrote:

>> 2) change TopologyLink to contain Ptr<Node> instead of a node name:
>
> this will
>>
>> save the user from having to manage the std::map<std::string,
>
> TopologyNode >
>
> Your proposal is indeed logical, but it does contain hidden nightmares.
> Let me explain.
> You need a list of the nodes and their attributes, and you need also to
> quick access to them with an index. The most logical index would be the
> node's pointer, but you can't made a map with the node pointer as first
> index, as it would be a mess due to how the map works. So the node
> "name" is the logical index.
>
> The best thing is to add the node pointers to the TopologyLink, so the
> user doesn't really need to go trough the map. However the map is stil
> needed, otherwise accessing the node attributes will require a search
> trough a vector or a list, which is not really user-friendy. I know that
> at the moment node attributes aren't used at all, but they're there to
> hold AS annotations, which is a really important feature. Anyway, now
> the TopologyLink now holds both the node name and the node pointer.
> Redundant, but it does the trick.

The key, I think, is that you need you separate implementation from
public API. i.e., I have no problem with you using an std::map which
contains names internally in ::Read but I think that the public API
will be best served if you do not export that: it should not be too
hard to convert your std::map<names> into a std::vector<nodes> at end
of ::Read and store the latter in your class so that you can ...

> Point 4 leaves me a bit puzzled. We can define some iterators, but the
> TopologyLink and TopologyNode structures are "owned" by the calling
> code. The readers just have references to them, but it doesn't hold them
> in private variables. To have iterators I should move those structures
> to the reader. It can be done of course, but it would require some work.

.. move the structures to the reader. In general, it's best to avoid
passing around structures across your API because that saves you from
having to deal with lifetime/ownership issues.

> I'll think about it tomorrow before posting the new version. However I
> feel that doing so we'll hide too much stuff to the calling function.

I read carefully your example code: it is just iterating over the data
structures so, it should be trivial to rework it to using the
iterators.

> By the way, while a TopologyLinkIterator is useful, as you'll scan all
> the link sequentially to build up the network, a NodeIterator isn't
> useful for real, as it's a map. The "normal" operation sequence (when
> we'll have AS annotations ofc) is:
> - pick a link
> - make the node interfaces
> - make the link itself
> - assing IP numbers based on the nodes annotations
> For the last point you don't need an iterator. That's why I used a map.
> It seems a bit weird at first, but it isn't. AS annotation is really a
> property of the node, not the link, so to decide the IP number you have
> to check both link endpoints and see if both belongs to the same AS or
> not. Hence, why the users should iterate over that map ?

I think that you need to provide a bit more context about this usecase
so that we can understand what you need to do. Do you have sample code
assigning these IP addresses using ASes ?

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

Tommaso Pecorella

Hi Mathieu, I'll try to address your points, that indeed are good points, mind. I ...

15 years, 2 months ago (2010-03-01 10:21:53 UTC) #19

Hi Mathieu,

I'll try to address your points, that indeed are good points, mind.

I don't like the map too much either. I don't like the whole idea of needing a
map. But it's the best I can do without limiting the user.

At the moment no topology generator is providing the AS data. Orbis is expected
to have it in the next release, but tbh I don't want to delay the code. On the
other hand we can't foresee all the possible topology generators data
structures, and that's why we need maximum flexibility. Multi-tier generators
could export tier information, AS data, whatever. We do not know and we can not
limit the user.

Now, if you look carefully at the map and why it's there, you can easily spot
the ratio of the whole thing. And the real answer is: nodes and tags.

IF (I repeat, if) a node could have a tag like a packet do, the problem would be
solved, no more need of any other clumsy structure. You could scan the links,
have the nodes, inspect the nodes for tags, done. Easy. Point is: nodes doesn't
have tags.

If you want I can borrow the tag structures from the packets and add them to the
nodes, and that would solve the problem once and for all. However to do that the
following have to be done:
1) get the tags class and clean up them, as right now they're used only by
packets and their structure is kinda "tailored" for that.
2) change the node class in order to add a taglist to it.
3) add a set of APIs to the node related to tags, able to:
3a) inspect, add, remove a NodeTag
3b) create a container out of another given a set of NodeTags to be matched

If I do that (and mind, it's not a one-night work) we can forget the map thing
and everything else. All the user will need is the links and the nodes
container. Too bad we can't do a similar thing for the links tho, it would had
been great. But there's no "LinkContainer" idea in ns-3. Not a biggie, we can
live with it.

Now the question is:
shall we delay the topology reader until I do that ? It might miss the ns-3.8
deadline.

On the other hand, since it's a "contrib" code, we could also put it in the main
code under contrib and, upon fixing the node tags, move it in a proper place. It
should not be an issue from the users point of view, as (from the wiki):
"Please note that the src/contrib/ directory in ns-3 is a place for experimental
code or code that the maintainers have not yet committed to accepting into the
main tree. Some ns-3 code (such as statistics code) started out by living in the
src/contrib/ directory while it was being evaluated. This is an alternate target
for contributing your code."

So we could simply add it right now (in 3.8) to the contrib section, as the user
should know that the code might change its APIs in the near future.

That would also allow some feedback from the users and (maybe) some more
topology readers.

Your call. If you think the NodeTag work is a good idea and we should do that,
I'll start working on it as soon as possible.

Best regards,

Tommaso

On 01/mar/2010, at 10.38, Mathieu Lacage wrote:

> On Sat, Feb 27, 2010 at 4:08 AM,  <tommypec@gmail.com> wrote:
> 
>>> 2) change TopologyLink to contain Ptr<Node> instead of a node name:
>> 
>> this will
>>> 
>>> save the user from having to manage the std::map<std::string,
>> 
>> TopologyNode >
>> 
>> Your proposal is indeed logical, but it does contain hidden nightmares.
>> Let me explain.
>> You need a list of the nodes and their attributes, and you need also to
>> quick access to them with an index. The most logical index would be the
>> node's pointer, but you can't made a map with the node pointer as first
>> index, as it would be a mess due to how the map works. So the node
>> "name" is the logical index.
>> 
>> The best thing is to add the node pointers to the TopologyLink, so the
>> user doesn't really need to go trough the map. However the map is stil
>> needed, otherwise accessing the node attributes will require a search
>> trough a vector or a list, which is not really user-friendy. I know that
>> at the moment node attributes aren't used at all, but they're there to
>> hold AS annotations, which is a really important feature. Anyway, now
>> the TopologyLink now holds both the node name and the node pointer.
>> Redundant, but it does the trick.
> 
> The key, I think, is that you need you separate implementation from
> public API. i.e., I have no problem with you using an std::map which
> contains names internally in ::Read but I think that the public API
> will be best served if you do not export that: it should not be too
> hard to convert your std::map<names> into a std::vector<nodes> at end
> of ::Read and store the latter in your class so that you can ...
> 
>> Point 4 leaves me a bit puzzled. We can define some iterators, but the
>> TopologyLink and TopologyNode structures are "owned" by the calling
>> code. The readers just have references to them, but it doesn't hold them
>> in private variables. To have iterators I should move those structures
>> to the reader. It can be done of course, but it would require some work.
> 
> .. move the structures to the reader. In general, it's best to avoid
> passing around structures across your API because that saves you from
> having to deal with lifetime/ownership issues.
> 
>> I'll think about it tomorrow before posting the new version. However I
>> feel that doing so we'll hide too much stuff to the calling function.
> 
> I read carefully your example code: it is just iterating over the data
> structures so, it should be trivial to rework it to using the
> iterators.
> 
>> By the way, while a TopologyLinkIterator is useful, as you'll scan all
>> the link sequentially to build up the network, a NodeIterator isn't
>> useful for real, as it's a map. The "normal" operation sequence (when
>> we'll have AS annotations ofc) is:
>> - pick a link
>> - make the node interfaces
>> - make the link itself
>> - assing IP numbers based on the nodes annotations
>> For the last point you don't need an iterator. That's why I used a map.
>> It seems a bit weird at first, but it isn't. AS annotation is really a
>> property of the node, not the link, so to decide the IP number you have
>> to check both link endpoints and see if both belongs to the same AS or
>> not. Hence, why the users should iterate over that map ?
> 
> I think that you need to provide a bit more context about this usecase
> so that we can understand what you need to do. Do you have sample code
> assigning these IP addresses using ASes ?
> 
> Mathieu
> -- 
> Mathieu Lacage <mathieu.lacage@gmail.com>

Tommaso Pecorella

After a discussion with Mathieu (thanks again) I decided that the whole node attributes thing ...

15 years, 2 months ago (2010-03-02 17:34:44 UTC) #21

Tommaso Pecorella

I think this it more or less complete. The iterators are there (only one since ...

15 years, 2 months ago (2010-03-02 22:50:46 UTC) #23

Mathieu Lacage

comments below. http://codereview.appspot.com/204055/diff/7045/8056 File src/contrib/topology-read/topology-reader.h (right): http://codereview.appspot.com/204055/diff/7045/8056#newcode54 src/contrib/topology-read/topology-reader.h:54: std::map<std::string, std::string > linkAttr; I really do ...

15 years, 2 months ago (2010-03-04 08:09:01 UTC) #24

comments below.

http://codereview.appspot.com/204055/diff/7045/8056
File src/contrib/topology-read/topology-reader.h (right):

http://codereview.appspot.com/204055/diff/7045/8056#newcode54
src/contrib/topology-read/topology-reader.h:54: std::map<std::string,
std::string > linkAttr;
I really do not like the idea of exporting the std::map here. I guess the
easiest way to do this would be to make that a full-blown class with public
accessors and private members.

i.e., 

class TopologyReader
{
public:
  // note: it's an inner class.
  class Link 
  {
  public:
    Link (...);
    Ptr<Node> GetFromNode (void) const;
    Ptr<Node> GetToNode (void) const;
    ...
    // assert if input name is not defined
    std::string GetAttribute (std::string name);
    // return false if input name is not defined.
    bool GetAttributeFailSafe (std::string name, std::string &value);
    Iterator AttributesBegin (void);
    Iterator AttributesEnd (void);
  };

};

http://codereview.appspot.com/204055/diff/7045/8056#newcode92
src/contrib/topology-read/topology-reader.h:92: virtual int32_t Read
(NodeContainer &nodes) = 0;
It would be simpler to make Read return the NodeContainer. i.e.,

NodeContainer Read (void) = 0;

http://codereview.appspot.com/204055/diff/7045/8056#newcode97
src/contrib/topology-read/topology-reader.h:97: void SetTopoFileName (const
std::string fileName);
SetFileName (Topo is already in the class name)

http://codereview.appspot.com/204055/diff/7045/8056#newcode102
src/contrib/topology-read/topology-reader.h:102: std::string GetTopoFileName ()
const;
same as above.

http://codereview.appspot.com/204055/diff/7045/8056#newcode107
src/contrib/topology-read/topology-reader.h:107: typedef std::list< TopologyLink
>::iterator TopologyLinksIterator;
It's best to separate the type declarattions to the top of the class
declaration. 

Also, rename to LinksIterator

http://codereview.appspot.com/204055/diff/7045/8056#newcode112
src/contrib/topology-read/topology-reader.h:112: typedef std::list< TopologyLink
>::const_iterator ConstTopologyLinksIterator;
Do you really need to provide both Const and non-const iterators ? I suspect
that you need only the Const variety.

http://codereview.appspot.com/204055/diff/7045/8056#newcode117
src/contrib/topology-read/topology-reader.h:117: TopologyLinksIterator
TopologyLinksBegin (void);
LinksBegin: Topology is in the class name

http://codereview.appspot.com/204055/diff/7045/8056#newcode137
src/contrib/topology-read/topology-reader.h:137: int TopologyLinksSize (void)
const;
rename LinkSize

http://codereview.appspot.com/204055/diff/7045/8056#newcode142
src/contrib/topology-read/topology-reader.h:142: bool TopologyLinksEmpty (void)
const;
same as above.

Tommaso Pecorella

What can I say beside the fact that I agree? I like to be pushed ...

15 years, 2 months ago (2010-03-05 02:23:54 UTC) #26

What can I say beside the fact that I agree? I like to be pushed to do the
things in the "right" way, it saves from my inner laziness to do 'em quick and
dirty.

On 2010/03/04 08:09:01, Mathieu Lacage wrote:

> I really do not like the idea of exporting the std::map here. I guess the
> easiest way to do this would be to make that a full-blown class with public
> accessors and private members.

There you go.

> http://codereview.appspot.com/204055/diff/7045/8056#newcode92
> src/contrib/topology-read/topology-reader.h:92: virtual int32_t Read
> (NodeContainer &nodes) = 0;
> It would be simpler to make Read return the NodeContainer. i.e.,
> 
> NodeContainer Read (void) = 0;

It was also one of my first ideas, then abandoned when Read had two (3 at one
point) structures to fill.

Now there's only the node container, so no point in not doing that. The lazy
user can guess a reading error by checking the node container (empty) and / or
the links number (zero) or even using the LinksEmpty().

> http://codereview.appspot.com/204055/diff/7045/8056#newcode97
> [snip: various comments about the namings]

Done. As a matter of fact it's useless to triple state that it's Topology stuff.

> http://codereview.appspot.com/204055/diff/7045/8056#newcode107
> src/contrib/topology-read/topology-reader.h:107: typedef std::list<
TopologyLink
> >::iterator TopologyLinksIterator;
> It's best to separate the type declarattions to the top of the class
> declaration. 

moved.

> http://codereview.appspot.com/204055/diff/7045/8056#newcode112
> src/contrib/topology-read/topology-reader.h:112: typedef std::list<
TopologyLink
> >::const_iterator ConstTopologyLinksIterator;
> Do you really need to provide both Const and non-const iterators ? I suspect
> that you need only the Const variety.

At first I was thinking to leave the capability to manually insert links to the
topology. Hence the non-const iterators. However... why should a user would like
to do that? If they need they can just add the links to the simulation.
No need, removed.

Best regards,

Tommaso

Josh Pelkey

Mathieu, With merge deadline looming, could you have a look at Tommaso's updated patch set?

15 years, 1 month ago (2010-03-08 19:54:44 UTC) #27

Mathieu Lacage

More comments. Mostly minor. This is pushing the merge deadline a bit... http://codereview.appspot.com/204055/diff/10001/11007 File src/contrib/attribute-default-iterator.cc ...

15 years, 1 month ago (2010-03-09 07:43:15 UTC) #28

Tommaso Pecorella

On 2010/03/09 07:43:15, Mathieu Lacage wrote: > More comments. Mostly minor. This is pushing the ...

15 years, 1 month ago (2010-03-09 18:58:41 UTC) #30

On 2010/03/09 07:43:15, Mathieu Lacage wrote:
> More comments. Mostly minor. This is pushing the merge deadline a bit...

Yes it is. To be honest I didn't expect more comments, especially over some code
you already checked. Over the new one maybe, not over what has been there from
the start.

> http://codereview.appspot.com/204055/diff/10001/11007#oldcode11
> src/contrib/attribute-default-iterator.cc:11:
> AttributeDefaultIterator::~AttributeDefaultIterator ()
> this whole change appears to be accidental coding style changes. If so, you
need
> to split this from your patch.

I said earlier that it was a wrong push due to oversensitive check-style.py.
It's kinda obvious I wouldn't push this file.

> http://codereview.appspot.com/204055/diff/10001/11011#newcode62
> src/contrib/topology-read/orbis-topology-reader.h:62: virtual NodeContainer
Read
> ( void );
> same as before. No spaces.

Minor, done on all files.

> http://codereview.appspot.com/204055/diff/10001/11012#newcode43
> src/contrib/topology-read/topology-reader.cc:43: NS_LOG_FUNCTION_NOARGS ();
> It's best to do this instead:
> NS_LOG_FUNCTION (this);

Same.

> http://codereview.appspot.com/204055/diff/10001/11013#newcode46
> src/contrib/topology-read/topology-reader.h:46: class TopologyReader : public
> Object
> This class appears to be intended to be used directly from the main script of
a
> user. If so, I would suggest that you do not derive from Object (hence, no
> GetTypeId, no Dispose) because user-visible objects are supposed to be
> instanciable on the Stack while Object-derived objects cannot be instanciated
on
> the stack. So, it's a helper class except that the 'Helper' keyword is not in
> its name.

This topic has been discussed in some previous patch. And I already stated that
this ain't a class to be used directly from the user. You need an helper to use
it or some carefully coding.
I already said I didn't wrote an helper on purpose since the code is in /contrib
and I was going to write it when (and if) the code will go out of contrib
status.
On the other hand some other classes in contrib have the very same issue, so I
don't see it as a major issue.
The point is: the class is not an helper in disguise. The helper is deliberately
missing. If you feel that the user is so dumb to not be able to see the example
code and we have to protect them even from that... well, I don't want to meet
that user to be honest.

> http://codereview.appspot.com/204055/diff/10001/11013#newcode125
> src/contrib/topology-read/topology-reader.h:125: Link ()
> no code in headers

Funny, there was some more code above and you didn't see it. Nvm, I removed
both.

> http://codereview.appspot.com/204055/diff/10001/11013#newcode156
> src/contrib/topology-read/topology-reader.h:156: virtual NodeContainer Read (
> void ) = 0;
> Read () or Read (void), not Read ( void )

Corrected.

> http://codereview.appspot.com/204055/diff/10001/11013#newcode197
> src/contrib/topology-read/topology-reader.h:197: 
> add a private keyword here and an AddLink method before:
> void AddLink (Link link); And doxygen the AddLink method

Done.

Now, I did everything but I have to confess that I'm a bit disappointed, since
all but one comment are about the previous patch, or even the one before. As a
teacher I'm used to comment everything at once, not bit by bit. This to prevent
infinite loops, if you know what I mean.

However I addressed (I hope) your last points. If there are no more issues this
patch can be applied. Or not.

We're already behind the 3.8 schedule and I'm in Kracow for an EU project
meeting (I'm the technical leader, can't avoid the meetings), so I can't access
my primary computer where I have all the code and, most important, the scripts
to push the repository.

If all the reviewers feels that this is good enough to be pushed, I'd ask one of
you to be so kind to push it for me, as I'll be home only saturday and that's
really too late for 3.8.

If we choose to not go for the 3.8 release, I'll do it when I'll be back home.
However I want to remind that my goal for 3.8 integration wasn't born out of
nowhere, but was driven by GSOC approaching. Since one of the (possible) tasks
in the next GSOC is about Topology systems, I felt idiotic to ask the applicants
to look at ns-3 AND to this patch.

Best regards,

Tommaso

tomh_tomh.org

> If we choose to not go for the 3.8 release, I'll do it when ...

15 years, 1 month ago (2010-03-10 06:36:09 UTC) #32

Mathieu Lacage

On Tue, Mar 9, 2010 at 7:58 PM, <tommypec@gmail.com> wrote: > Yes it is. To ...

15 years, 1 month ago (2010-03-10 08:00:02 UTC) #33

On Tue, Mar 9, 2010 at 7:58 PM,  <tommypec@gmail.com> wrote:
> Yes it is. To be honest I didn't expect more comments, especially over
> some code you already checked. Over the new one maybe, not over what has
> been there from the start.

I did not read the rest of the code before because I was waiting until
the public API was fixed.

>
>> http://codereview.appspot.com/204055/diff/10001/11007#oldcode11
>> src/contrib/attribute-default-iterator.cc:11:
>> AttributeDefaultIterator::~AttributeDefaultIterator ()
>> this whole change appears to be accidental coding style changes. If
>
> so, you need
>>
>> to split this from your patch.
>
> I said earlier that it was a wrong push due to oversensitive
> check-style.py. It's kinda obvious I wouldn't push this file.

I don't think I read your earlier reply and if I did, I forgot about
it. It would be simpler to remove it to avoid confusion. If there is a
bit you don't expect to push, don't include it in your patch: really,
I have zero contextual memory when I read a patch.

>> http://codereview.appspot.com/204055/diff/10001/11013#newcode46
>> src/contrib/topology-read/topology-reader.h:46: class TopologyReader :
>
> public
>>
>> Object
>> This class appears to be intended to be used directly from the main
>
> script of a
>>
>> user. If so, I would suggest that you do not derive from Object
>
> (hence, no
>>
>> GetTypeId, no Dispose) because user-visible objects are supposed to be
>> instanciable on the Stack while Object-derived objects cannot be
>
> instanciated on
>>
>> the stack. So, it's a helper class except that the 'Helper' keyword is
>
> not in
>>
>> its name.
>
> This topic has been discussed in some previous patch. And I already
> stated that this ain't a class to be used directly from the user. You

I am afraid but I don't understand this: how do you expect the user to
use this code ? The example code in
examples/topology-read/topology-example-sim.cc does use it directly
and that is what I looked at. How do you expect the user to use this
code ? It might be that I missed a comment from you in a previous
reply. Is that so ?

> need an helper to use it or some carefully coding.
> I already said I didn't wrote an helper on purpose since the code is in
> /contrib and I was going to write it when (and if) the code will go out
> of contrib status.

I am afraid but I don't see the causal relationship between having a
helper class and being located in src/contrib.

Anyway, my point is not that a helper is missing. My point is that
this class appears to have both the role and the semantics of a helper
except that it derives from Object so, all you need to do to make this
class integrate seamlessly with the rest of the ns-3 helper API is to
remove that Object base class and that's it: I don't think that this
is related in any way with which directory it is put in or which name
it has. Is this problematic ?

> On the other hand some other classes in contrib have the very same
> issue, so I don't see it as a major issue.
> The point is: the class is not an helper in disguise. The helper is
> deliberately missing. If you feel that the user is so dumb to not be
> able to see the example code and we have to protect them even from
> that... well, I don't want to meet that user to be honest.

I probably have missed something but which example code are you talking about ?

> Now, I did everything but I have to confess that I'm a bit disappointed,
> since all but one comment are about the previous patch, or even the one
> before. As a teacher I'm used to comment everything at once, not bit by
> bit. This to prevent infinite loops, if you know what I mean.

I usually make high-level comments, wait until they are fixed, and,
then, get into the details. I am sure it's not perfect and this
process can be improved: complaining about it is clearly the right
thing to do if you feel it's not ok: I never claimed I was always
right.

> However I addressed (I hope) your last points. If there are no more
> issues this patch can be applied. Or not.
>
> We're already behind the 3.8 schedule and I'm in Kracow for an EU
> project meeting (I'm the technical leader, can't avoid the meetings), so
> I can't access my primary computer where I have all the code and, most
> important, the scripts to push the repository.
>
> If all the reviewers feels that this is good enough to be pushed, I'd
> ask one of you to be so kind to push it for me, as I'll be home only
> saturday and that's really too late for 3.8.
>
> If we choose to not go for the 3.8 release, I'll do it when I'll be back
> home. However I want to remind that my goal for 3.8 integration wasn't
> born out of nowhere, but was driven by GSOC approaching. Since one of
> the (possible) tasks in the next GSOC is about Topology systems, I felt
> idiotic to ask the applicants to look at ns-3 AND to this patch.

It up to our release manager to decide what to include or not. I don't
care much either way: I am merely a review monkey.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

Tommaso Pecorella

Hi Tom, it's not a big issue after all. It will give me the time ...

15 years, 1 month ago (2010-03-10 11:52:31 UTC) #34

Hi Tom,

it's not a big issue after all. It will give me the time to:
1) write that figgin' helpers (and maybe streamline more the APIs)
2) maybe Obis 0.8 will come out, and then I'll have some work to do for real.
3) fix one of the biggest use-cases I left out so far.

We just need to make sure that if a GSOC student will have the bad idea to
choose the topology topic, he/she is pointed here.

Anyway, here is what I'll work on, so you'll all know.

So far the code is focused on a rather simple task: read a topology from a file
and build it in ns-3. So far this is useful, but not SO useful. I mean, a user
could use it as it is, but probably they'll end up with a completely wrong
topological system.

Let me start from scratch, and this is code and feature design.

We all know that Internet and networks in general are made by Autonomous Systems
(ASs), joined together.

We have some topology generators able to create an AS-level topology. Some
others are more focused on internal-AS topologies, some others are LAN-like.

Now, why I started this code and what was my issue. I wanted to do some
experiments with heavily-sparse networks, as is P2P networks where the clients
could belong to different ASs. So I needed an AS-level topology. I guess that
this is the most general use-case. I believe that the users could want to do
that, otherwise I'd had kept this private.

As it is, is the actual code a solution to my problem? Not yet.

What's missing?
Internal-AS topology (multi-tier topology support) -or- a decent statistical
distribution for inter-AS delays.

Let me explain with an example:
1) you build up the topology with the actual code. All the nodes are ASs border
routers.
2) you have to choose ASs and put into them some random nodes (your application
nodes).

Point 2 is actually impossible, as if you connect them with random nodes created
in point 1 you'll miss the internal-AS details. You have to create a sub-network
"internal" to the AS (a node from point 1) and place there your nodes.

Is this possible with the topology system right now? Not in a direct way.
Is this obvious? To anyone with a basic topology issues knowledge, yes. To the
rest of the wold, no.

I didn't add this support yet because it's covering exactly what is asked in the
GSOC topic, so doing so would burn a topic. Nevertheless, it have to be done. At
this point, since the 3.8 deadline is obviously gone, I'll freeze this for a
couple of weeks. If the topic will be chosen for GSOC, we'll have a worker.
Otherwise I'll do it. I already spotted an article on arXiv about this and some
possible solutions.

Be prepared, as the next patch might include quite important changes also to
other ns-3 elements, such as nodes. So I'll probably split it in 2 different
patches, one being a prerequisite for the other one.

This of course assuming that the patch will not be accepted as it is. I
addressed (I hope) all the comments done so far but the ones about helpers. On
the other hand I have to admit that more work can be done. I'm not pushing for
merging with 3.8 to be honest, I just didn't want to miss the opportunity if
there was a general consensus. If there isn't, not a biggie. The patch will be
bigger, huger and more complete for 3.9.

If (and I'll stress again, I'm not pushing for it against reviewer's opinion,
which I value the most) there is a consensus about merging it as it is, all the
above things are the working plan for expansion of this code toward 3.9.

Finally, I'd like to thank again the reviewers, and in particular Mathieu. I can
disagree with him on some things, but he really helped to improve the code
design.

Best regards,

Tommaso

On 10/mar/2010, at 07.36, Tom Henderson wrote:

> 
>> If we choose to not go for the 3.8 release, I'll do it when I'll be back
>> home. However I want to remind that my goal for 3.8 integration wasn't
>> born out of nowhere, but was driven by GSOC approaching. Since one of
>> the (possible) tasks in the next GSOC is about Topology systems, I felt
>> idiotic to ask the applicants to look at ns-3 AND to this patch.
>> 
> 
> I'm surprised to see the door close on this contribution, since it is self
contained and going into src/contrib, and seems to have resolved all of the
comments.  I would support merging this still for 3.8.
> 
> Tom

Josh Pelkey

> I'm surprised to see the door close on this contribution, since it is > ...

15 years, 1 month ago (2010-03-10 13:00:27 UTC) #35

Josh Pelkey

15 years, 1 month ago (2010-03-12 19:53:38 UTC) #37

I just merged this with ns-3-dev: changeset b5bc10de166d

Thanks Tommaso!

Expand All Messages | Collapse All Messages