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

Issue 4940041: HTTP Traffic Generator

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 1 month ago by ycheng
Modified:
6 years, 11 months ago
Reviewers:
Tom Henderson, tomh, Nicola Baldo
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

The implementation of HTTP traffic generator composes mainly three parts: 1. Client 2. Server 3. Runtime Variable 4. Distribution Generation (Notification to Statistics Research, Bell Labs, Lucent Technologies and The University of North Carolina at Chapel Hill) When the traffic generator starts, the http client initialize connections to http server, after the connection has established, the client calls the distribution generation module and generates empirical parameters involved with http operation. For example, request size, response size, request gap time, server delay time, total objects per page, total pages per web session. All those parameters will be saved in runtime variable model, which can be accessed by both the http client and server. Based on the parameters saved in runtime variable, the client and server will generate http traffic which can represent real-world http traffic. This traffic generator is capable of generating both HTTP 1.0 traffic as well as HTTP 1.1 traffic. The detailed description can be found in in docs folder.

Patch Set 1 #

Total comments: 23

Patch Set 2 : updated based on Tom's review. #

Total comments: 29

Patch Set 3 : update http controller #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5004 lines, -0 lines) Patch
A src/http/docs/http.h View 1 1 chunk +43 lines, -0 lines 0 comments Download
A src/http/docs/http.rst View 1 2 1 chunk +109 lines, -0 lines 0 comments Download
A src/http/examples/http.cc View 1 2 1 chunk +132 lines, -0 lines 0 comments Download
A src/http/examples/wscript View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
A src/http/helper/http-helper.h View 1 2 1 chunk +174 lines, -0 lines 0 comments Download
A src/http/helper/http-helper.cc View 1 2 1 chunk +143 lines, -0 lines 0 comments Download
A src/http/model/http-client.h View 1 2 1 chunk +251 lines, -0 lines 0 comments Download
A src/http/model/http-client.cc View 1 2 1 chunk +733 lines, -0 lines 0 comments Download
A src/http/model/http-controller.h View 1 2 1 chunk +259 lines, -0 lines 0 comments Download
A src/http/model/http-controller.cc View 1 2 1 chunk +457 lines, -0 lines 0 comments Download
A src/http/model/http-distribution.h View 1 2 1 chunk +125 lines, -0 lines 0 comments Download
A src/http/model/http-distribution.cc View 1 2 1 chunk +272 lines, -0 lines 0 comments Download
A src/http/model/http-random-variable.h View 1 2 1 chunk +574 lines, -0 lines 0 comments Download
A src/http/model/http-random-variable.cc View 1 2 1 chunk +980 lines, -0 lines 0 comments Download
A src/http/model/http-server.h View 1 2 1 chunk +137 lines, -0 lines 0 comments Download
A src/http/model/http-server.cc View 1 2 1 chunk +316 lines, -0 lines 0 comments Download
A src/http/test/http-test-suite.cc View 1 2 1 chunk +260 lines, -0 lines 0 comments Download
A src/http/wscript View 1 2 1 chunk +33 lines, -0 lines 0 comments Download

Messages

Total messages: 2
Tom Henderson
There seems to be a lot of good work here, providing what appears to be ...
8 years, 7 months ago (2012-03-02 02:04:08 UTC) #1
Nicola Baldo
8 years, 2 months ago (2012-07-25 09:00:43 UTC) #2
I agree with Tom that this is a potentially very good contributions, but it
needs more work before it meets the quality standard required for merging with
ns-3-dev.

I reviewed Patch Set 2, I think that many points raised by Tom are still open:

- a lot of doxygen is missing

- there are still many coding style issues (e.g., correct
capitalization/CamelCase)

- the .rst doc section about testing are deceptive: you make a lot of claims
about testing and validation, but when one actually goes and looks into the
testing code it emerges that only a handful of tests are really meaningful, most
are useless (e.g., what's the purpose of testing the value of a constant?) and
overall there is minimal functional test coverage and no validation.

- the statistical model should be described in detail: the code is full of const
values for the various distribution, but there is no hint on where they come
from.

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst
File src/http/docs/http.rst (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode45
src/http/docs/http.rst:45: 
Any detailed info on the statistical model that is implemented in the generator?
This is the most important piece of information that one would expect to appear
here.

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode70
src/http/docs/http.rst:70: To have a node run HTTP, the easiest way would be to
use the ClickInternetStackHelper
ClickInternetStackHelper???

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode87
src/http/docs/http.rst:87: Validation
I would call this just "Testing".

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode92
src/http/docs/http.rst:92: * Unit tests have been written to verify the
internals of DSR. This can be found in ``src/http/test/http-test-suite.cc``.
These tests verify whether the methods inside DSR module which deal with packet
buffer, headers work correctly.
DSR?

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode93
src/http/docs/http.rst:93: * Simulation cases similar to [1] have been tested
and have comparable results.
"have comparable results" with what? [1] seems to describe the same model that
is being reviewed here, and I see no comparison with other data there.

http://codereview.appspot.com/4940041/diff/5021/src/http/docs/http.rst#newcode94
src/http/docs/http.rst:94: * Random variable validation cases have been
conducted to fit the proposed distribution function for each http parameter
this sentence is not clear, and what you do is not evident from the test
program. Can you please explain better?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
File src/http/model/http-distribution.h (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:58: namespace http {
do you really need a separate namespace? most of ns3 code just sits in the ns3
namespace.

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:77: inline double GetRate ()
from the ns-3 coding style: "Do not include inline implementations in header
files; put all implementation in a .cc file (unless implementation in the header
file brings demonstrable and significant performance improvement)."

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:81: // Get the request size
please use doxygen style for these comments. Applies to all the other methods
below.

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:82: uint32_t GetReqsize ();
coding style: should be GetReqSize, maybe better GetRequestSize

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:84: uint32_t GetRspsize ();
coding style: GetResponseSize

note from the coding style doc: "A long descriptive name which requires a lot of
typing is always better than a short name which is hard to decipher. Do not use
abbreviations in names unless the abbreviation is really unambiguous and obvious
to everyone (e.g., use "size" over "sz")"

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:101: double GetReqgap (uint32_t page,
uint32_t obj);
GetReqestGap

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:103: uint32_t AdjustPersistRspsz ();
AdjustPersistResponseSize

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:105: void ResetPersistRspsz ();
ResetPersistResponseSize

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-distribut...
src/http/model/http-distribution.h:115: HTTPFlowArriveRandomVariableImpl*
m_flowarriveRV;
coding style: 

HttpFlowArriveRandomVariableImpl m_flowArriveRandomVariable;

or just

HttpFlowArriveRandomVariableImpl m_flowArrive;

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
File src/http/model/http-random-variable.h (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:68: struct arima_params
ArimaParams

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:73: uint32_t pAR, qMA;
doxygen?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:81: class HTTPRandomVariableBase : public
RandomVariable
HttpRandomVariableBase

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:95: double logit (double m_x);
please use camel case for this and below methods
and add doxygen for each method

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:114:
/************************************************************************************************************************/
?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:115: class FX
any more meaningful name?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:139: class ARFIMAImpl : public
HTTPRandomVariableBase
ArfimaImpl

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:182: class
HTTPFlowArriveRandomVariableImpl : public HTTPRandomVariableBase
doxygen?

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:216: ///\name Fields
kill dead code

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:265: ///\name Fields
kill dead code

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:342: static struct arima_params
filesize_arima_params;
camel case

http://codereview.appspot.com/4940041/diff/5021/src/http/model/http-random-va...
src/http/model/http-random-variable.h:344: static FX fsize_invcdf[2][2];
camel case

http://codereview.appspot.com/4940041/diff/5021/src/http/test/http-test-suite.cc
File src/http/test/http-test-suite.cc (right):

http://codereview.appspot.com/4940041/diff/5021/src/http/test/http-test-suite...
src/http/test/http-test-suite.cc:205: NS_TEST_EXPECT_MSG_EQ
(http::HTTPFileSizeRandomVariableImpl::FSIZE1_PROB_A, 0.5, "trivial");
testing const values??? Come on, this is absolutely useless!!!

http://codereview.appspot.com/4940041/diff/5021/src/http/test/http-test-suite...
src/http/test/http-test-suite.cc:326: NS_TEST_EXPECT_MSG_EQ (rt.m_reqSize,  500,
"trivial");
are you testing that the assignment operator works properly?
Sign in to reply to this message.

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