Issue 289550043: TraceReplayApplication: new application layer model for replaying a pcap file in ns-3.24
|
Created:
8 years, 1 month ago by Prakash Agrawal Modified:
6 years, 6 months ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
DescriptionTraceReplayApplication: new application layer model for replaying a pcap file
The goal of TraceReplayApplication is to make simulations more realistic. We use network trace collected from user to infer and replay only application layer delays like user think times. TraceReplayApplication extracts application layer characteristics from a single trace, and replays this information across many users in simulation, by using suitable randomization.
TraceReplayApplication: takes a pcap as input and initializes all client-server pairs.
TraceReplayClient: implementation of client.
TraceReplayServer: implementation of server.
TraceReplay.cc: sample file for TraceReplayApplication. Takes pcap, number of client and random number as input. Replays pcap for each client using proper randomization.
TraceReplay.py and TraceReplay.sh: scripts used by TraceReplayApplication to internally convert a pcap into formatted trace file which can be used by application.
*Reference:
Presented at the NCC 2016.
https://goo.gl/Z4ZW2K
Patch Set 1 #Patch Set 2 : Modified TraceReplay patch for ns-3.24 #
Total comments: 39
Patch Set 3 : Modified TraceReplay patch for ns-3.25 #
Total comments: 13
Patch Set 4 : Modified TraceReplay patch for ns-3.25 #
Total comments: 6
Patch Set 5 : Modified TraceReplay patch for ns-3.25 #
Total comments: 88
Patch Set 6 : Modified TraceReplay patch for ns-3.25 #
Total comments: 26
MessagesTotal messages: 25
Hello all, The goal of TraceReplayApplication is to make simulations more realistic. We use network trace collected from user to infer and replay only application layer delays like user think times. TraceReplayApplication extracts application layer characteristics from a single trace, and replays this information across many users in simulation, by using suitable randomization. This patch add three new classes: TraceReplayApplication: takes a pcap as input and initializes all client-server pairs. TraceReplayClient: implementation of client. TraceReplayServer: implementation of server. The sample file TraceReplay.cc can be run by following command: ./waf --run "scratch/traceReplay --pcapPath=scratch/pcap_name --R=random_number --nWifi=number_of_wifi_client” pcapPath: path to input pcap file collected at single client R: random number nWifi: number of wifi clients that user wants to simulate The sample file will create a wireless setup described in file and replay the input pcap for each client. The patch is against ns-3-dev, changeset 12055. Please review this at https://codereview.appspot.com/289550043/ Thank you for considering patch for a review process, Prakash Agarwal
Sign in to reply to this message.
I have filed this issue in our tracker so that we do not forget to review this. Thank you for submitting. https://www.nsnam.org/bugzilla/show_bug.cgi?id=2342
Sign in to reply to this message.
Hi, I just downloaded the model and did a quick check. here's my first comments. First and foremost, let me say that the approach is really interesting and that I'd really like to include it in ns-3. However, i have some doubts about the code structure. The first one is the TraceReplayApplication. This looks more like an Helper than an Application. I'd strongly suggest to consider its refactoring into an Helper. I'll not go into the small code bits right now, like the direct use of "srand", this will be addressed in the next review. The second part is about the use of python scripts and scratch folder. I'm ok with python, but in *this* particular case it seems unnecessary. The whole script can be easily made in C++. The advantage is to avoid a dependency on python (and on python versions). Moreover, the code in the "src" folder shouldn't use the one in "scratch". Accessory classes and default files should be either in the model or in the examples directory (the ones inside the module folder). The last point is: documentation please. Given the particularity of the model, I feel that a good documentation is extremely important. In particular the role of the server and client: so far we have little or no cases of servers reacting to client data - with the exceptions of Echo. In *this* case it's really different. As a consequence, we need a clear documentation explaining the use case and the limitations.
Sign in to reply to this message.
Hello Sir, Thank you very much for your feedback on the code. I am working on the documentation and python to cpp code conversion. I will update the patch soon. I am not able to completely understand the first point in you comment (about refactoring TraceReplayApplication into a Helper). Could you please explain in a bit detail? The main purpose of the TraceReplayApplication is to read trace file and initialize all the connections between client and server. Is it ok if we do this in a Helper? Thanks and regards, Prakash
Sign in to reply to this message.
On 2016/04/06 08:42:49, Prakash Agrawal wrote: > Hello Sir, > > Thank you very much for your feedback on the code. > > I am working on the documentation and python to cpp code conversion. I will > update the patch soon. > I am not able to completely understand the first point in you comment (about > refactoring TraceReplayApplication into a Helper). Could you please explain in a > bit detail? The main purpose of the TraceReplayApplication is to read trace file > and initialize all the connections between client and server. Is it ok if we do > this in a Helper? > > Thanks and regards, > Prakash Hi, in this particular case, I'd say that yes, a Helper would be the right choice. After all, the class does "help" the other applications (server and client) in the initialization phase. As a consequence, it's an Helper. Cheers, T.
Sign in to reply to this message.
Hello Sir, I have updated the trace-replay patch. I have tried to incorporate all you previous suggestions in this patch. Thank you for your guidance. Any suggestions / corrections on the updated patch would be much appreciated. Thanks and regards, Prakash
Sign in to reply to this message.
Second batch of comments. I didn't complete the review because I think we'll need a slight refactoring, and I'd like to review the next iteration as a whole. Cheers, T. PS: you're using tabs instead of spaces for the indentation. Fortunately there's a script in utils. https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... File examples/trace-replay/trace-replay-example.cc (right): https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ Where's its wscript ? ## -*- Mode: python; py-indent-offset: 4; indent-tabs-mode: nil; coding: utf-8; -*- def build(bld): obj = bld.create_ns3_program('trace-replay-example', ['point-to-point', 'wifi', 'internet', 'applications']) obj.source = 'trace-replay-example.cc' https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:25: // ./waf --run "scratch/trace-replay-example --pcapPath=scratch/pcap.pcapng --R=0 --nWifi=1" remember to add the pcap file to the examples directory and to fix the path here https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:166: } It is a comment about the helper, but it's easier to show it here. Instead of this organization, which is error-prone, try using this one: // ptr = new SharedPacCount[nWifi]; // unnecessary, create it when instilling in the node pair. for (uint32_t i = 0; i < nWifi; i++) { TraceReplayHelper application(DataRate ("25MBps")); application.SetPcap (pcapPath); // or, this is mutually exclusive. application.SetTraceFile (tracePath); application.SetStartTime (something); application.SetStartTime (somethingMore); application.Install (wifiInterfaces.GetAddress (i), wifiStaNodes.Get (i), p2pInterfaces.GetAddress (1), p2pNodes.Get (1)); } Rationale for this... avoid things that the user shouldn't know / don't want to remember / are internal things. About the new and missing stuff. - SetRandomNum is unnecessary (see the comments in the helper) - SetPcap could be skipped if you provide a trace file (but validate it). If both are set, then the trace file should be always overwritten. - SetClient and SetRemote don't really respect the usual Install system. Usually we do an install on a node, or on a node container. In this case you must have a pair of nodes and their addresses. it's ok, but it's better to have a single instruction (well, it's a matter of personal feeling). - Start - well, this *is* the Install. Name it Install. I'd add a SetStartTime, i.e., the time at which the stream is meant to start (if provided). If nothing is provided, then the data can start at the offset found in the pcap. In this way you can have a better control of the simulation. For example you can use a trace but add like 30 seconds to initialize a routing algorithm. https://codereview.appspot.com/289550043/diff/20001/src/applications/doc/trac... File src/applications/doc/trace-replay.rst (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/doc/trac... src/applications/doc/trace-replay.rst:8: The goal of TraceReplay is to make simulations more realistic. TraceReplay uses network trace collected from user to infer and replay only application layer delays like user think times. TraceReplay extracts application layer characteristics from a single trace, and replays this information across many users in simulation, by using suitable randomization. I'd strongly suggest to use your paper as a reference, e.g., "by using a suitable randomization. For further details see [Paper]_" ... References ========== .. [Paper] add references to your paper. Note: if your paper hasn't been (yet) published you could add it as unpublished, internal report or as you like better. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:34: m_pacCountPtr = &pacCountPtr; Unnecessary, see below. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:559: } If the pcap file is missing but you already have the trace file, then you don't need the pcap file, right ? Fix please. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:577: } Here you assume that the file has a particular name and a particular format, but you don't triple check the format. What happens if the file is modified by the user ? Suggestions: 1) Allow to specify a trace file name (and skip the pcap parsing), and 2) Do a robust trace file parser (e.g., comments are all the lines starting with a #) https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:100: void SetRemoteAddress (Ipv4Address add, Ptr<Node> node); Both SetClient and SetRemote should be able to use Ipv6Address as well. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:119: uint32_t m_randomNum; //!< Random number to randomize the delays Is it really necessary ? It seems that it's used as a binary choice (%2). As a consequence this value is not "dramatic". On the opposite, use the random streams assignments to set the random generators. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:120: uint32_t m_id; //!< Id of the client What is this used for ? From the code, it seems unnecessary (it is even used uninitialized, thus set to zero). https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:127: SharedPacCount *m_pacCountPtr; //!< Pointer to SharedPacCount class Avoid "normal" pointers. Use Ptr instead. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:127: SharedPacCount *m_pacCountPtr; //!< Pointer to SharedPacCount class Some of these variables (e.g., m_randomNum) are not initialized in the constructor. You must take care of non-initialized variables before using them. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... File src/applications/model/trace-replay-utility.h (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:36: * Any connection can see the total packet send by other connection, total packet sent* https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:40: class SharedPacCount Expand the class name and 1) make it self-explanatory plus 2) explain clearly its goals in the doxygen. Remember that the users will crawl the classes and don't know where a class is used. Another alternative is to make it general enough to be used by anything, and modify the documentation accordingly. In other terms, this class seems to be "only" a simple key-based packet counter, not much more than a map. As a side note, do you really need to have a class for this purpose ? In your example, the variable is shared among all the servers belonging to the same nodes pair (client-server pair). If this is the intended use, you could use a simple map< pair <uint16_t, uint16_t > > Another alternative is: don't use it at all. Each ServerApp can ask all the other server apps for this data in real time. for (uint32_t index=0; index<GetNode ()->GetNApplications (); index++) { Ptr <TraceReplayServer> app = DynamicCast <TraceReplayServer> (GetNode ()->GetApplication (index)); if (app) { // ask the packet counter ... Of course this is slow[er]. *However*, You can do the app scanning during DoInitialize and memorize the pointers. In this way it will be almost as fast as having a shared counter, without the shared counter. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:49: * \returns Send packet count for the connections I'd not use strings (see below). You can use InetSocketAddress and Inet6SocketAddress. Otherwise, if you want to keep the class ip-version free, use Address, uint16_t pairs (i.e., generic Address plus a port number). https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:63: void Initialize (std::pair<std::pair<std::string, std::string>, std::pair<std::string, std::string> > key); In the above functions the 1st parameter is not used, remove it. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:141: struct m_parallelConn don't name a struct m_something. Name it ParallelConnectionInfo or something similar. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:144: std::string m_dstPort; //!< server's port number of parallel connection Why are you storing these as stings ? https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:148: long double m_delay; //!< Delay of packet if it's a Time, use the Time class.
Sign in to reply to this message.
On 2016/05/02 17:42:29, KGang790 wrote:
Sign in to reply to this message.
Hello Sir, Thank you for your comments. I have updated the trace-replay patch. I have tried to incorporate all you suggestions in this patch (except for robust trace file parsing). Any suggestions / corrections on the updated patch would be much appreciated. Thanks and regards, Prakash https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... File examples/trace-replay/trace-replay-example.cc (right): https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > Where's its wscript ? > > ## -*- Mode: python; py-indent-offset: 4; indent-tabs-mode: nil; coding: utf-8; > -*- > > def build(bld): > obj = bld.create_ns3_program('trace-replay-example', ['point-to-point', > 'wifi', 'internet', 'applications']) > obj.source = 'trace-replay-example.cc' > Done. https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:25: // ./waf --run "scratch/trace-replay-example --pcapPath=scratch/pcap.pcapng --R=0 --nWifi=1" On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > remember to add the pcap file to the examples directory and to fix the path here Done. https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:166: } On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > It is a comment about the helper, but it's easier to show it here. > > Instead of this organization, which is error-prone, try using this one: > // ptr = new SharedPacCount[nWifi]; // unnecessary, create it when instilling > in the node pair. > for (uint32_t i = 0; i < nWifi; i++) > { > TraceReplayHelper application(DataRate ("25MBps")); > application.SetPcap (pcapPath); > // or, this is mutually exclusive. > application.SetTraceFile (tracePath); > application.SetStartTime (something); > application.SetStartTime (somethingMore); > application.Install (wifiInterfaces.GetAddress (i), wifiStaNodes.Get (i), > p2pInterfaces.GetAddress (1), p2pNodes.Get (1)); > } > > Rationale for this... avoid things that the user shouldn't know / don't want to > remember / are internal things. > > About the new and missing stuff. > - SetRandomNum is unnecessary (see the comments in the helper) > - SetPcap could be skipped if you provide a trace file (but validate it). If > both are set, then the trace file should be always overwritten. > - SetClient and SetRemote don't really respect the usual Install system. Usually > we do an install on a node, or on a node container. In this case you must have a > pair of nodes and their addresses. it's ok, but it's better to have a single > instruction (well, it's a matter of personal feeling). > - Start - well, this *is* the Install. Name it Install. > > I'd add a SetStartTime, i.e., the time at which the stream is meant to start (if > provided). > If nothing is provided, then the data can start at the offset found in the pcap. > In this way you can have a better control of the simulation. For example you can > use a trace but add like 30 seconds to initialize a routing algorithm. Hello Sir, The reason for not using SetStartTime () is because a client might have many connections associated with it. With SetStartTime () all these connections will start at same time which might not be desired. To give better control of the simulation, as you have suggested, we can add the time provided by SetStartTime () to the offset found in the pcap ? https://codereview.appspot.com/289550043/diff/20001/src/applications/doc/trac... File src/applications/doc/trace-replay.rst (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/doc/trac... src/applications/doc/trace-replay.rst:8: The goal of TraceReplay is to make simulations more realistic. TraceReplay uses network trace collected from user to infer and replay only application layer delays like user think times. TraceReplay extracts application layer characteristics from a single trace, and replays this information across many users in simulation, by using suitable randomization. On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > I'd strongly suggest to use your paper as a reference, e.g., "by using a > suitable randomization. For further details see [Paper]_" > > ... > > References > ========== > > .. [Paper] add references to your paper. > > Note: if your paper hasn't been (yet) published you could add it as unpublished, > internal report or as you like better. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:34: m_pacCountPtr = &pacCountPtr; On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > Unnecessary, see below. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:559: } On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > If the pcap file is missing but you already have the trace file, then you don't > need the pcap file, right ? > Fix please. > Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:577: } On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > Here you assume that the file has a particular name and a particular format, but > you don't triple check the format. > What happens if the file is modified by the user ? > > Suggestions: > 1) Allow to specify a trace file name (and skip the pcap parsing), and > 2) Do a robust trace file parser (e.g., comments are all the lines starting with > a #) Hello SIr, Could you please explain a bit more on robust file parser ? Will regular expression matching on each lines will do ? https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:100: void SetRemoteAddress (Ipv4Address add, Ptr<Node> node); On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > Both SetClient and SetRemote should be able to use Ipv6Address as well. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:119: uint32_t m_randomNum; //!< Random number to randomize the delays On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > Is it really necessary ? > It seems that it's used as a binary choice (%2). As a consequence this value is > not "dramatic". > On the opposite, use the random streams assignments to set the random > generators. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:120: uint32_t m_id; //!< Id of the client On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > What is this used for ? > From the code, it seems unnecessary (it is even used uninitialized, thus set to > zero). Hello Sir, In TraceReplay we assume that all the clients connect to a single server. m_id is unique id given to each client, so that each client connect to a different port in server. Eg. portNumber = (m_id*numConn + j) + 49153; If we have 2 clients with 10 connections each, client-1 (m_id = 0) will connect to port numbers 49153 - 49162 and client-2 (m_id = 1) will connect to port numbers 49163 - 49172. It is set in TraceReplayHelper (uint32_t id, DataRate dataRate, double stopTime) { m_id = id; m_stopTime = Seconds (stopTime); m_dataRate = dataRate; m_randomNum = 0; NS_LOG_FUNCTION (this); } https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:127: SharedPacCount *m_pacCountPtr; //!< Pointer to SharedPacCount class On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > Some of these variables (e.g., m_randomNum) are not initialized in the > constructor. You must take care of non-initialized variables before using them. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:127: SharedPacCount *m_pacCountPtr; //!< Pointer to SharedPacCount class On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > Avoid "normal" pointers. Use Ptr instead. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... File src/applications/model/trace-replay-utility.h (right): https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:36: * Any connection can see the total packet send by other connection, On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > total packet sent* Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:40: class SharedPacCount On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > Expand the class name and 1) make it self-explanatory plus 2) explain clearly > its goals in the doxygen. > Remember that the users will crawl the classes and don't know where a class is > used. > > Another alternative is to make it general enough to be used by anything, and > modify the documentation accordingly. > In other terms, this class seems to be "only" a simple key-based packet counter, > not much more than a map. > > As a side note, do you really need to have a class for this purpose ? > In your example, the variable is shared among all the servers belonging to the > same nodes pair (client-server pair). > If this is the intended use, you could use a simple map< pair <uint16_t, > uint16_t > > > > Another alternative is: don't use it at all. Each ServerApp can ask all the > other server apps for this data in real time. > for (uint32_t index=0; index<GetNode ()->GetNApplications (); index++) > { > Ptr <TraceReplayServer> app = DynamicCast <TraceReplayServer> (GetNode > ()->GetApplication (index)); > if (app) > { > // ask the packet counter > ... > > Of course this is slow[er]. > *However*, You can do the app scanning during DoInitialize and memorize the > pointers. In this way it will be almost as fast as having a shared counter, > without the shared counter. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:49: * \returns Send packet count for the connections On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > I'd not use strings (see below). You can use InetSocketAddress and > Inet6SocketAddress. Otherwise, if you want to keep the class ip-version free, > use Address, uint16_t pairs (i.e., generic Address plus a port number). Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:63: void Initialize (std::pair<std::pair<std::string, std::string>, std::pair<std::string, std::string> > key); On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > In the above functions the 1st parameter is not used, remove it. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:141: struct m_parallelConn On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > don't name a struct m_something. Name it ParallelConnectionInfo or something > similar. Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:144: std::string m_dstPort; //!< server's port number of parallel connection On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > Why are you storing these as stings ? Done. https://codereview.appspot.com/289550043/diff/20001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:148: long double m_delay; //!< Delay of packet On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > if it's a Time, use the Time class. Done.
Sign in to reply to this message.
https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... File examples/trace-replay/trace-replay-example.cc (right): https://codereview.appspot.com/289550043/diff/20001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:25: // ./waf --run "scratch/trace-replay-example --pcapPath=scratch/pcap.pcapng --R=0 --nWifi=1" On 2016/05/05 10:38:56, Prakash Agrawal wrote: > On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > > remember to add the pcap file to the examples directory and to fix the path > here > Hello Sir, I am not able to add the pcap file using upload.py.
Sign in to reply to this message.
Hi, thanks for the upgrades, I'll check the code as soon as possible. In the meantime, I'll reply to your questions. On 2016/05/05 10:38:56, Prakash Agrawal wrote: > Hello Sir, > > The reason for not using SetStartTime () is because a client might have many > connections associated with it. With SetStartTime () all these connections will > start at same time which might not be desired. To give better control of the > simulation, as you have suggested, we can add the time provided by SetStartTime > () to the offset found in the pcap ? That's a good point, and a good solution. > https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... > src/applications/helper/trace-replay-helper.cc:577: } > On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > > Here you assume that the file has a particular name and a particular format, > but > > you don't triple check the format. > > What happens if the file is modified by the user ? > > > > Suggestions: > > 1) Allow to specify a trace file name (and skip the pcap parsing), and > > 2) Do a robust trace file parser (e.g., comments are all the lines starting > with > > a #) > > Hello SIr, > > Could you please explain a bit more on robust file parser ? Will regular > expression matching on each lines will do ? Yes, but using a regular expression is probably not necessary. Just check that each line has the right number of parameters, and skip the lines starting with a #. In this way you can add comments to the trace file, e.g., the source pcap you used, the time the trace was generated and so on. It might seems unnecessary, but when you try to figure out where the trace came from after a couple of months, you'll be happy to have a comment. > https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... > src/applications/helper/trace-replay-helper.h:120: uint32_t m_id; > //!< Id of the client > On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > > What is this used for ? > > From the code, it seems unnecessary (it is even used uninitialized, thus set > to > > zero). > > Hello Sir, > In TraceReplay we assume that all the clients connect to a single server. m_id > is unique id given to each client, so that each client connect to a different > port in server. > Eg. portNumber = (m_id*numConn + j) + 49153; > If we have 2 clients with 10 connections each, client-1 (m_id = 0) will connect > to port numbers 49153 - 49162 and client-2 (m_id = 1) will connect to port > numbers 49163 - 49172. > > It is set in > TraceReplayHelper (uint32_t id, DataRate dataRate, double stopTime) > { > m_id = id; > m_stopTime = Seconds (stopTime); > m_dataRate = dataRate; > m_randomNum = 0; > NS_LOG_FUNCTION (this); > } I see. In this case TraceReplayHelper should declare that variable as static, initialize it to zero and increment it for each client *automatically*. However, you're missing one use-case. What if I use *two* traces (one for Type A clients and one for Type B clients) and each trace has a different number of connections ? Moreover, what if we want to use multiple servers ? One idea could be to simply pass to the client the starting port number. You'll have the same effect, and way more flexibility. Cheers, T. PS: don't worry about the pcap. However, it would be great if you could sent it by e-mail (if it's not too big).
Sign in to reply to this message.
Hello Sir, A gentle reminder for posting the review. Kind Regards, Prakash Agrawal On 2016/05/10 22:46:27, Tommaso Pecorella wrote: > Hi, > > thanks for the upgrades, I'll check the code as soon as possible. In the > meantime, I'll reply to your questions. > > On 2016/05/05 10:38:56, Prakash Agrawal wrote: > > Hello Sir, > > > > The reason for not using SetStartTime () is because a client might have many > > connections associated with it. With SetStartTime () all these connections > will > > start at same time which might not be desired. To give better control of the > > simulation, as you have suggested, we can add the time provided by > SetStartTime > > () to the offset found in the pcap ? > > That's a good point, and a good solution. > > > > https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... > > src/applications/helper/trace-replay-helper.cc:577: } > > On 2016/04/23 17:39:28, Tommaso Pecorella wrote: > > > Here you assume that the file has a particular name and a particular format, > > but > > > you don't triple check the format. > > > What happens if the file is modified by the user ? > > > > > > Suggestions: > > > 1) Allow to specify a trace file name (and skip the pcap parsing), and > > > 2) Do a robust trace file parser (e.g., comments are all the lines starting > > with > > > a #) > > > > Hello SIr, > > > > Could you please explain a bit more on robust file parser ? Will regular > > expression matching on each lines will do ? > > Yes, but using a regular expression is probably not necessary. > Just check that each line has the right number of parameters, and skip the lines > starting with a #. > In this way you can add comments to the trace file, e.g., the source pcap you > used, the time the trace was generated and so on. > It might seems unnecessary, but when you try to figure out where the trace came > from after a couple of months, you'll be happy to have a comment. > > > > https://codereview.appspot.com/289550043/diff/20001/src/applications/helper/t... > > src/applications/helper/trace-replay-helper.h:120: uint32_t m_id; > > > //!< Id of the client > > On 2016/04/23 17:39:29, Tommaso Pecorella wrote: > > > What is this used for ? > > > From the code, it seems unnecessary (it is even used uninitialized, thus set > > to > > > zero). > > > > Hello Sir, > > In TraceReplay we assume that all the clients connect to a single server. m_id > > is unique id given to each client, so that each client connect to a different > > port in server. > > Eg. portNumber = (m_id*numConn + j) + 49153; > > If we have 2 clients with 10 connections each, client-1 (m_id = 0) will > connect > > to port numbers 49153 - 49162 and client-2 (m_id = 1) will connect to port > > numbers 49163 - 49172. > > > > It is set in > > TraceReplayHelper (uint32_t id, DataRate dataRate, double stopTime) > > { > > m_id = id; > > m_stopTime = Seconds (stopTime); > > m_dataRate = dataRate; > > m_randomNum = 0; > > NS_LOG_FUNCTION (this); > > } > > I see. In this case TraceReplayHelper should declare that variable as static, > initialize it to zero and increment it for each client *automatically*. > However, you're missing one use-case. > What if I use *two* traces (one for Type A clients and one for Type B clients) > and each trace has a different number of connections ? > Moreover, what if we want to use multiple servers ? > > One idea could be to simply pass to the client the starting port number. You'll > have the same effect, and way more flexibility. > > Cheers, > > T. > > PS: don't worry about the pcap. However, it would be great if you could sent it > by e-mail (if it's not too big).
Sign in to reply to this message.
On 2016/05/26 09:29:29, Prakash Agrawal wrote: > Hello Sir, > > A gentle reminder for posting the review. > > Kind Regards, > Prakash Agrawal Hi, sorry for the delay. overall I'd say that we're almost there. I have a last group of comments, they should be easy to fix. most of them apply to more than one place, but I'm confident that you'll extend them to al the codebase. As a side note, I liked a lot the way you solved the problem of tracking multiple connections from (potentially) multiple app instances.
Sign in to reply to this message.
https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:593: } My previous comments was about this (skip the first 35 lines). I'd suggest the following strategy instead. done = false do { std::string line; getline (infile, line); if (line starts with #, skip) else read "numConn from line and set done to true } while !done In the next for, do the same. If you find a line with a leading #, skip it. https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:611: double startTime; You're loosing precision. Here and where you read the pcap (and write the trace). PCAP files can have (just) two time precisions. You should find out what the pcap is using and save the time as integers in the trace file. Read back as integers and convert them to Time. This also means that the trace file format should be expanded to include the time precision, e.g., "-3" could mean that the integer represents time in milliseconds. Note: you just need to support two different time precisions: the ones you find in pcap. Forget the doubles when you compute time, everywhere. https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:65: * \brief This method sets the input the pcap file As below, explain briefly that the pcap will be ignored if a trace file is found. https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:72: * \brief This method sets the input the trace file Explain what the trace file is used for (also in the .rst documentation) As an example: "TraceReplay will use the trace file (if found). Else it will create the TraceFile from the supplied PCAP file". https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... File src/applications/model/trace-replay-client.cc (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:131: } Clever! I love it. https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:160: TraceReplayClient::SetConnId (std::string ipClient, uint16_t portClient, std::string ipServer, uint16_t portServer) Why don't you pass the IP numbers as Addresses ? Moreover, there's an implicit assumption that the address is IPv4, which is limiting. I'd fix this in the Helper, and pass Address. Here you can use Ipv4Address::IsMatchingType to do the right thing. https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... File src/applications/model/trace-replay-client.h (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... src/applications/model/trace-replay-client.h:111: * \returns uint16_t port number of server in the original connection no need to indicate the return unit (and it's wrong in the following functions)
Sign in to reply to this message.
https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:593: } On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > My previous comments was about this (skip the first 35 lines). > I'd suggest the following strategy instead. > > done = false > do { > std::string line; > getline (infile, line); > if (line starts with #, skip) > else read "numConn from line and set done to true > } while !done > > In the next for, do the same. > If you find a line with a leading #, skip it. Done. https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:611: double startTime; Hello Sir, I am not able to follow your point. I read the wireshark manual (https://www.wireshark.org/docs/wsug_html_chunked/ChAdvTimestamps.html) but was not able to find the time formats. Could you please give me some reference. I have made all the other changes and will upload new patch along with this. Kind regards, Prakash On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > You're loosing precision. Here and where you read the pcap (and write the > trace). > > PCAP files can have (just) two time precisions. You should find out what the > pcap is using and save the time as integers in the trace file. Read back as > integers and convert them to Time. This also means that the trace file format > should be expanded to include the time precision, e.g., "-3" could mean that the > integer represents time in milliseconds. > > Note: you just need to support two different time precisions: the ones you find > in pcap. > > Forget the doubles when you compute time, everywhere. https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:65: * \brief This method sets the input the pcap file On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > As below, explain briefly that the pcap will be ignored if a trace file is > found. Done. https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:72: * \brief This method sets the input the trace file On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > Explain what the trace file is used for (also in the .rst documentation) > As an example: "TraceReplay will use the trace file (if found). Else it will > create the TraceFile from the supplied PCAP file". Done. https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... File src/applications/model/trace-replay-client.cc (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:160: TraceReplayClient::SetConnId (std::string ipClient, uint16_t portClient, std::string ipServer, uint16_t portServer) On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > Why don't you pass the IP numbers as Addresses ? > Moreover, there's an implicit assumption that the address is IPv4, which is > limiting. > I'd fix this in the Helper, and pass Address. > Here you can use Ipv4Address::IsMatchingType to do the right thing. Done. https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... File src/applications/model/trace-replay-client.h (right): https://codereview.appspot.com/289550043/diff/40001/src/applications/model/tr... src/applications/model/trace-replay-client.h:111: * \returns uint16_t port number of server in the original connection On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > no need to indicate the return unit (and it's wrong in the following functions) Done.
Sign in to reply to this message.
On 2016/05/27 15:40:02, Prakash Agrawal wrote: > > https://codereview.appspot.com/289550043/diff/40001/src/applications/helper/t... > src/applications/helper/trace-replay-helper.cc:611: double startTime; > Hello Sir, > > I am not able to follow your point. I read the wireshark manual > (https://www.wireshark.org/docs/wsug_html_chunked/ChAdvTimestamps.html) but was > not able to find the time formats. Could you please give me some reference. > > I have made all the other changes and will upload new patch along with this. > > Kind regards, > Prakash > > On 2016/05/26 22:51:01, Tommaso Pecorella wrote: > > You're loosing precision. Here and where you read the pcap (and write the > > trace). > > > > PCAP files can have (just) two time precisions. You should find out what the > > pcap is using and save the time as integers in the trace file. Read back as > > integers and convert them to Time. This also means that the trace file format > > should be expanded to include the time precision, e.g., "-3" could mean that > the > > integer represents time in milliseconds. > > > > Note: you just need to support two different time precisions: the ones you > find > > in pcap. > > > > Forget the doubles when you compute time, everywhere. Hi Prakash, I have read your code (again) and maybe I found the problem. I thought you was reading the pcap file directly, not though tshark. If tshark writes the packet time in seconds (thus, a double), you're right in reading it as a double. Just convert it as soon as possible into a Time and document the various randomisation you do (I have seen that they have been changed, and I will not object, it's your model). Cheers, T.
Sign in to reply to this message.
Hello Sir, Thank you for your comments. I have updated the trace-replay patch. Any suggestions / corrections on the updated patch would be much appreciated. Thanks and regards, Prakash
Sign in to reply to this message.
Hi, there are my last ones. I guess that after fixing them this is ready for merge (if nobody has other comments). Cheers, T. https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:206: getline (tmp1, ipClient, ':'); I don't know what is the tshark output format, but here's a potential issue. ":" is used in IPv6 addresses, so this will go crazy with an IPv6 address. https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:215: std::cerr << "Number of clients in pcap file > 1.\n"; use NS_ASSERT_MSG https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:93: void SetRandomNum (int r); Don't send me to the hell, please... Why don't you use a "normal" random generation system ? See for example the "RequestJitter" attribute in ArpL3Protocol. In any case, it should be clarified what the effect is, e.g., start the flow x seconds (?) later.
Sign in to reply to this message.
Hello Sir, Thank you for your guidance. I have updated the TraceReplay patch incorporating all your suggestions. Thanks and regards, Prakash https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:206: getline (tmp1, ipClient, ':'); On 2016/05/29 16:12:55, Tommaso Pecorella wrote: > I don't know what is the tshark output format, but here's a potential issue. ":" > is used in IPv6 addresses, so this will go crazy with an IPv6 address. Done. https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... src/applications/helper/trace-replay-helper.cc:215: std::cerr << "Number of clients in pcap file > 1.\n"; On 2016/05/29 16:12:54, Tommaso Pecorella wrote: > use NS_ASSERT_MSG Done. https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/60001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:93: void SetRandomNum (int r); On 2016/05/29 16:12:55, Tommaso Pecorella wrote: > Don't send me to the hell, please... > Why don't you use a "normal" random generation system ? > See for example the "RequestJitter" attribute in ArpL3Protocol. > > In any case, it should be clarified what the effect is, e.g., start the flow x > seconds (?) later. Done.
Sign in to reply to this message.
Hello, here are a set of comments and suggestions; I would like to review again once the code is revised. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... File examples/trace-replay/trace-replay-example.cc (right): https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:20: * Refrence: https://goo.gl/Z4ZW2K again, see my comment elsewhere about removing this URL https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:25: // ./waf --run "scratch/trace-replay-example --pcapPath=example/trace-replay/trace-replay-sample.pcap --nWifi=1" update this comment; it won't run from the scratch directory if it is merged to ns-3-dev https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:28: // nWifi : Number of wifi client to simulate what happens if the number of wifi clients requested exceeds the number of clients in the pcap file? https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:42: // corresponding TraceReplayServer, for each connection. Can you describe a bit more here about how the pcap file is used. What happens if user tries to use a different pcap file? Are there conditions on the file that need to be satisfied? https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:73: cmd.AddValue ("R", "Random variable", R); this variable 'R' appears to be unused https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:77: long double stopTime = 1000; why is this a long double type? https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:157: // application.SetTraceFile (traceFilePath); remove commented out lines in your final code https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/wsc... File examples/trace-replay/wscript (right): https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/wsc... examples/trace-replay/wscript:4: obj = bld.create_ns3_program('trace-replay-example', consider to put this example into the src/applications/examples directory instead https://codereview.appspot.com/289550043/diff/80001/src/applications/doc/trac... File src/applications/doc/trace-replay.rst (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/doc/trac... src/applications/doc/trace-replay.rst:76: can greatly enhance the realistic nature of simulation results. Please add comment on the properties of network trace files that are required as inputs. https://codereview.appspot.com/289550043/diff/80001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:92: void SetRandomStream (int64_t stream); For API consistency, suggest to change this to int64_t AssignStreams (int64_t stream); https://codereview.appspot.com/289550043/diff/80001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:231: double CalPacketDelay (uint32_t frameNum, bool timeOut, bool httpReq, double currTime, double packetTime); CalculatePacketDelay() https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-client.cc (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:256: // No more packets to recieve. Close connection receive https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:281: Time tNext (Seconds (packet.GetDelay () + packet.GetSize () * 8 / static_cast<double> (m_dataRate.GetBitRate ()))); It is better to avoid Time to double conversions if you can. Replace with Time tNext = packet.GetDelay () + m_dataRate.CalculateBytesTxTime (packet.GetSize ()); https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:282: m_sendEvent = Simulator::Schedule (tNext, &TraceReplayClient::SendPacket, this, packet); I suggest some NS_LOG_DEBUG or NS_LOG_LOGIC statements in this file for significant events such as (for example): NS_LOG_DEBUG ("Scheduling next event at time " << (Simulator::Now () + tNext)); https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:328: // keep recieving packet receiving https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:338: // No more packet to send or recieve receive https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-client.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:87: void SetConnId (Address ipClient, uint16_t portClient, Address ipServer, uint16_t portServer); SetConnectionId https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:105: Address GetIpServer (void); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:112: uint16_t GetPortServer (void); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:119: uint16_t GetPortClient (void); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:126: uint32_t GetTotByteCount (void); GetTotalByteCount (const) https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-server.cc (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.cc:137: // Two connections are considered parallel if both source and destiantion Ip's destination https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.cc:245: Time tNext (Seconds ((packet.GetSize () - socket->GetTxAvailable ()) * 8 / static_cast<double> (m_dataRate.GetBitRate ()))); see comment in TraceReplayClient about how to refactor the above statement. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.cc:272: // go to recieve mode receive https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-server.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:32: #include "ns3/core-module.h" Do not use '*-module.h' headers in either helper/ or model/ directory code. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:104: void SetConnId (Address ipClient, uint16_t portClient, Address ipServer, uint16_t portServer); SetConnectionId https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:109: * \param address local address how will this address be used-- is there any constraint on what underlying type it is (e.g. InetSocketAddress)? https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:110: * \param dataRate The datarate of the sever How does a server have 'datarate'? Elaborate on what this does. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:122: Address GetIpServer (void); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:129: uint16_t GetPortServer (void); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:136: uint16_t GetPortClient (void); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:143: uint32_t GetTotByteCount (void); GetTotalByteCount (void) const; https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-utility.cc (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.cc:20: * Refrence: https://goo.gl/Z4ZW2K Please do not include a hyperlink to a Dropbox document. This looks like it is a published IEEE paper. Instead, provide the IEEE paper reference. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-utility.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:27: #include "ns3/core-module.h" we do not use the module headers "core-module.h" etc. in other model files; these are reserved for user-level programs and examples. Use only the low level header files that you need. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:41: * \brief TraceReplayPacket is a packet container for TraceReplay. This doxygen is confusing to me. Is a TraceReplayPacket a container of packets? How do packets have parallel connections? Please elaborate a bit more here. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:58: uint32_t GetNumParalleConn (); suggest to spell out Connection instead of abbreviating as Conn(); also Parallel is misspelled https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:62: * \param i index for m_parallelConnList do not expose private member variable names in Doxygen; encapsulation should shield users from knowing these details. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:66: uint32_t GetByteCount (uint32_t i); should be const method? https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:75: std::pair<uint16_t, uint16_t> GetConnId (uint32_t i); const method? https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:87: void AddParallelConn (uint16_t srcPort, uint16_t dstPort, uint32_t byteCount); Change to AddParallelConnection https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:97: * \brief Set delay of TraceReplayPacket Please elaborate on how the 'delay' value is used by the model. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:108: uint32_t GetSize (); const https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:115: double GetDelay (); const. Return a Time object; do not convert to double unless you need to https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:125: uint32_t GetByteCount (uint16_t portClient, uint16_t portServer); const
Sign in to reply to this message.
Hello Sir, Thank you for comments on the code. I have updated the TraceReplay patch incorporating all your suggestions. Regards, Prakash https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... File examples/trace-replay/trace-replay-example.cc (right): https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:20: * Refrence: https://goo.gl/Z4ZW2K On 2016/06/25 05:29:06, Tom Henderson wrote: > again, see my comment elsewhere about removing this URL Done. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:25: // ./waf --run "scratch/trace-replay-example --pcapPath=example/trace-replay/trace-replay-sample.pcap --nWifi=1" On 2016/06/25 05:29:05, Tom Henderson wrote: > update this comment; it won't run from the scratch directory if it is merged to > ns-3-dev Done. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:28: // nWifi : Number of wifi client to simulate On 2016/06/25 05:29:06, Tom Henderson wrote: > what happens if the number of wifi clients requested exceeds the number of > clients in the pcap file? The number of clients in the pcap must be 1. nWifi could be any number that user wants to simulate. Optionally, users can also give different pcap as input to different nodes. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:42: // corresponding TraceReplayServer, for each connection. On 2016/06/25 05:29:06, Tom Henderson wrote: > > Can you describe a bit more here about how the pcap file is used. What happens > if user tries to use a different pcap file? Are there conditions on the file > that need to be satisfied? Done. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:73: cmd.AddValue ("R", "Random variable", R); On 2016/06/25 05:29:06, Tom Henderson wrote: > this variable 'R' appears to be unused Done. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:77: long double stopTime = 1000; On 2016/06/25 05:29:06, Tom Henderson wrote: > why is this a long double type? Done. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/tra... examples/trace-replay/trace-replay-example.cc:157: // application.SetTraceFile (traceFilePath); On 2016/06/25 05:29:06, Tom Henderson wrote: > remove commented out lines in your final code Done. https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/wsc... File examples/trace-replay/wscript (right): https://codereview.appspot.com/289550043/diff/80001/examples/trace-replay/wsc... examples/trace-replay/wscript:4: obj = bld.create_ns3_program('trace-replay-example', On 2016/06/25 05:29:06, Tom Henderson wrote: > consider to put this example into the src/applications/examples directory > instead Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/doc/trac... File src/applications/doc/trace-replay.rst (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/doc/trac... src/applications/doc/trace-replay.rst:76: can greatly enhance the realistic nature of simulation results. On 2016/06/25 05:29:06, Tom Henderson wrote: > > Please add comment on the properties of network trace files that are required as > inputs. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/helper/t... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:92: void SetRandomStream (int64_t stream); On 2016/06/25 05:29:06, Tom Henderson wrote: > For API consistency, suggest to change this to > int64_t AssignStreams (int64_t stream); Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/helper/t... src/applications/helper/trace-replay-helper.h:231: double CalPacketDelay (uint32_t frameNum, bool timeOut, bool httpReq, double currTime, double packetTime); On 2016/06/25 05:29:06, Tom Henderson wrote: > CalculatePacketDelay() Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-client.cc (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:256: // No more packets to recieve. Close connection On 2016/06/25 05:29:06, Tom Henderson wrote: > receive Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:281: Time tNext (Seconds (packet.GetDelay () + packet.GetSize () * 8 / static_cast<double> (m_dataRate.GetBitRate ()))); On 2016/06/25 05:29:06, Tom Henderson wrote: > It is better to avoid Time to double conversions if you can. Replace with > > Time tNext = packet.GetDelay () + m_dataRate.CalculateBytesTxTime > (packet.GetSize ()); Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:282: m_sendEvent = Simulator::Schedule (tNext, &TraceReplayClient::SendPacket, this, packet); On 2016/06/25 05:29:06, Tom Henderson wrote: > I suggest some NS_LOG_DEBUG or NS_LOG_LOGIC statements in this file for > significant events such as (for example): > NS_LOG_DEBUG ("Scheduling next event at time " << (Simulator::Now () + > tNext)); Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:328: // keep recieving packet On 2016/06/25 05:29:06, Tom Henderson wrote: > receiving Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.cc:338: // No more packet to send or recieve On 2016/06/25 05:29:06, Tom Henderson wrote: > receive Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-client.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:87: void SetConnId (Address ipClient, uint16_t portClient, Address ipServer, uint16_t portServer); On 2016/06/25 05:29:06, Tom Henderson wrote: > SetConnectionId Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:105: Address GetIpServer (void); On 2016/06/25 05:29:06, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:112: uint16_t GetPortServer (void); On 2016/06/25 05:29:06, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:119: uint16_t GetPortClient (void); On 2016/06/25 05:29:06, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-client.h:126: uint32_t GetTotByteCount (void); On 2016/06/25 05:29:06, Tom Henderson wrote: > GetTotalByteCount (const) Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-server.cc (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.cc:137: // Two connections are considered parallel if both source and destiantion Ip's On 2016/06/25 05:29:06, Tom Henderson wrote: > destination Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.cc:245: Time tNext (Seconds ((packet.GetSize () - socket->GetTxAvailable ()) * 8 / static_cast<double> (m_dataRate.GetBitRate ()))); On 2016/06/25 05:29:06, Tom Henderson wrote: > see comment in TraceReplayClient about how to refactor the above statement. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.cc:272: // go to recieve mode On 2016/06/25 05:29:06, Tom Henderson wrote: > receive Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-server.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:32: #include "ns3/core-module.h" On 2016/06/25 05:29:07, Tom Henderson wrote: > Do not use '*-module.h' headers in either helper/ or model/ directory code. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:104: void SetConnId (Address ipClient, uint16_t portClient, Address ipServer, uint16_t portServer); On 2016/06/25 05:29:07, Tom Henderson wrote: > SetConnectionId Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:109: * \param address local address On 2016/06/25 05:29:07, Tom Henderson wrote: > how will this address be used-- is there any constraint on what underlying type > it is (e.g. InetSocketAddress)? It is of type 'Address', to which server will bind and listen. Set by TraceReplayHelper. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:110: * \param dataRate The datarate of the sever On 2016/06/25 05:29:07, Tom Henderson wrote: > How does a server have 'datarate'? Elaborate on what this does. 'dataRate' is servers data rate while sending reply bytes to the client. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:122: Address GetIpServer (void); On 2016/06/25 05:29:07, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:129: uint16_t GetPortServer (void); On 2016/06/25 05:29:07, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:136: uint16_t GetPortClient (void); On 2016/06/25 05:29:07, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-server.h:143: uint32_t GetTotByteCount (void); On 2016/06/25 05:29:07, Tom Henderson wrote: > GetTotalByteCount (void) const; Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-utility.cc (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.cc:20: * Refrence: https://goo.gl/Z4ZW2K On 2016/06/25 05:29:07, Tom Henderson wrote: > Please do not include a hyperlink to a Dropbox document. This looks like it is > a published IEEE paper. Instead, provide the IEEE paper reference. Hello Sir, The paper is accepted and presented in NCC 2016 but is not published yet. Can we update the link after it is published ? Thanks and regards, Prakash https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... File src/applications/model/trace-replay-utility.h (right): https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:27: #include "ns3/core-module.h" On 2016/06/25 05:29:07, Tom Henderson wrote: > we do not use the module headers "core-module.h" etc. in other model files; > these are reserved for user-level programs and examples. Use only the low level > header files that you need. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:41: * \brief TraceReplayPacket is a packet container for TraceReplay. On 2016/06/25 05:29:07, Tom Henderson wrote: > This doxygen is confusing to me. Is a TraceReplayPacket a container of packets? > How do packets have parallel connections? Please elaborate a bit more here. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:58: uint32_t GetNumParalleConn (); On 2016/06/25 05:29:07, Tom Henderson wrote: > suggest to spell out Connection instead of abbreviating as Conn(); also Parallel > is misspelled Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:62: * \param i index for m_parallelConnList On 2016/06/25 05:29:07, Tom Henderson wrote: > do not expose private member variable names in Doxygen; encapsulation should > shield users from knowing these details. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:66: uint32_t GetByteCount (uint32_t i); On 2016/06/25 05:29:07, Tom Henderson wrote: > should be const method? Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:75: std::pair<uint16_t, uint16_t> GetConnId (uint32_t i); On 2016/06/25 05:29:07, Tom Henderson wrote: > const method? Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:87: void AddParallelConn (uint16_t srcPort, uint16_t dstPort, uint32_t byteCount); On 2016/06/25 05:29:07, Tom Henderson wrote: > Change to AddParallelConnection Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:97: * \brief Set delay of TraceReplayPacket On 2016/06/25 05:29:07, Tom Henderson wrote: > Please elaborate on how the 'delay' value is used by the model. Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:108: uint32_t GetSize (); On 2016/06/25 05:29:07, Tom Henderson wrote: > const Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:115: double GetDelay (); On 2016/06/25 05:29:07, Tom Henderson wrote: > const. Return a Time object; do not convert to double unless you need to Done. https://codereview.appspot.com/289550043/diff/80001/src/applications/model/tr... src/applications/model/trace-replay-utility.h:125: uint32_t GetByteCount (uint16_t portClient, uint16_t portServer); On 2016/06/25 05:29:07, Tom Henderson wrote: > const Done.
Sign in to reply to this message.
> https://goo.gl/Z4ZW2K > On 2016/06/25 05:29:07, Tom Henderson wrote: > > Please do not include a hyperlink to a Dropbox document. This looks like it > is > > a published IEEE paper. Instead, provide the IEEE paper reference. > > Hello Sir, > > The paper is accepted and presented in NCC 2016 but is not published yet. Can we > update the link after it is published ? > > Thanks and regards, > Prakash Yes, either provide a hyperlink to the IEEE Xplore citation once it becomes available, or else a link to an author-prepared preprint, if you prefer, that conforms to the IEEE copyright policy regarding such preprints (https://www.ieee.org/documents/author_faq.pdf).
Sign in to reply to this message.
Here is another batch of comments. My high-level comment is that this code appears to lack tests, and there is a lot of code in the trace relay helper including parsing and regex code that possibly could go wrong? Also, there is a process for distilling a trace from a pcap; has it been tested that the original pcap and the subsequent trace will yield the same output? Can you let us know how extensively (against how many pcap files) it has been tested against, and whether the pcap to trace process has been thoroughly tested? Can you provide an interesting enough reference trace that could be saved and used in a regression test, and if so, how could we automatically test that it was properly read and the right number of connections and packets were generated? https://codereview.appspot.com/289550043/diff/100001/src/applications/doc/tra... File src/applications/doc/trace-replay.rst (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/doc/tra... src/applications/doc/trace-replay.rst:10: simulation, by using suitable randomization. It seems to me that you should clearly state that this application is only TCP based (TCP traces) unless you want to generalize it further. https://codereview.appspot.com/289550043/diff/100001/src/applications/example... File src/applications/examples/trace-replay-example.cc (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/example... src/applications/examples/trace-replay-example.cc:20: * Refrence: https://goo.gl/Z4ZW2K This is still a Dropbox URL; please change. https://codereview.appspot.com/289550043/diff/100001/src/applications/example... src/applications/examples/trace-replay-example.cc:111: NqosWifiMacHelper mac = NqosWifiMacHelper::Default (); The NqosWifiMacHelper is deprecated now; use WifiMacHelper https://codereview.appspot.com/289550043/diff/100001/src/applications/example... src/applications/examples/trace-replay-example.cc:159: int64_t streamIndex = 1; https://codereview.appspot.com/289550043/diff/100001/src/applications/example... src/applications/examples/trace-replay-example.cc:165: application.AssignStreams (i); streamIndex += application.AssignStreams (streamIndex); (your AssignStreams (i) only works if AssignStreams assigns only one stream-- what if this changes someday?) https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.cc:20: * Refrence: https://goo.gl/Z4ZW2K delete URL reference https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.cc:63: NS_LOG_FUNCTION (this); place all NS_LOG_FUNCTION log statements at the top of the method, and include input parameters; e.g. NS_LOG_FUNCTION (this << dataRate); https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.cc:220: if (delay < 0.000001) In general, I would prefer to see use of ns3 Time objects instead of doubles; this includes also at the APIs https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.cc:228: TraceReplayHelper::ProcessPacket (m_connId id, uint32_t packetSize, double packetTime, uint32_t frameNum) this method could use some logging (NS_LOG_DEBUG) statements throughout. https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.cc:323: std::string filename = "tcpPackets.csv"; is this documented anywhere? (default csv filename) https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... File src/applications/helper/trace-replay-helper.h (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.h:20: * Refrence: https://goo.gl/Z4ZW2K Delete URL reference https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... File src/applications/model/trace-replay-client.cc (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.cc:20: * Refrence: https://goo.gl/Z4ZW2K delete URL reference https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.cc:199: // If the conneciton does not have any packet then numReq will be empty. s/conneciton/connection https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.cc:200: // Insert 0 to indicate client that it does not have to send any packet should comment be moved within braces? https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.cc:243: Time tNext (Seconds (0.00001)); the comment is not aligned with the code; is it 10 or 100 us? Suggest to use MicroSeconds () class instead of Seconds() https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... File src/applications/model/trace-replay-client.h (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.h:20: * Refrence: https://goo.gl/Z4ZW2K Delete URL reference https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.h:81: void SetConnectionId (Address ipClient, uint16_t portClient, Address ipServer, uint16_t portServer); we usually pass addresses in our APIs as references to const; e.g. 'const Address &ipClient') https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-client.h:188: uint32_t m_totRecByte; //!< Total number of bytes recieved so far s/recieved/received https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... File src/applications/model/trace-replay-server.cc (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-server.cc:20: * Refrence: https://goo.gl/Z4ZW2K Delete URL https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-server.cc:254: Time tNext (Seconds (0.00001)); similar comment as in the client code. https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... File src/applications/model/trace-replay-server.h (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-server.h:20: * Refrence: https://goo.gl/Z4ZW2K Delete URL https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-server.h:48: * In each cycle, it waits for client to send *m_expByte as request why do you say '*m_expByte'? can you state this more clearly without referencing internal variable names? https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... File src/applications/model/trace-replay-utility.cc (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-utility.cc:20: * Refrence: https://goo.gl/Z4ZW2K Delete URL https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... File src/applications/model/trace-replay-utility.h (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-utility.h:20: * Refrence: https://goo.gl/Z4ZW2K delete URL https://codereview.appspot.com/289550043/diff/100001/src/applications/model/t... src/applications/model/trace-replay-utility.h:39: * It is the idel time after which the packet was sent. s/idel/idle
Sign in to reply to this message.
https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... File src/applications/helper/trace-replay-helper.cc (right): https://codereview.appspot.com/289550043/diff/100001/src/applications/helper/... src/applications/helper/trace-replay-helper.cc:639: std::string line = CheckRegex (infile, std::regex ("^*\t[0-9]+\t*[0-9]+\t[0-9]+[.]?[0-9]*$")); This line is giving error:terminate called after throwing an instance of 'std::regex_error' correction to the code is :std::regex ("^.*\t[0-9]+\t.*[0-9]+\t[0-9]+[.]?[0-9]*$")
Sign in to reply to this message.
|
