New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NDT client: first iteration #441

Merged
merged 182 commits into from May 23, 2016

Conversation

Projects
None yet
3 participants
@bassosimone
Member

bassosimone commented Apr 4, 2016

This branch has now reached the "ready for review state". It contains the initial implementation of the NDT client, already good enough to start testing with it and already tested enough (coverage super high for all files of this pull request) to stay in master. Issue #400 highlights future improvements that could be implemented as separate pull requests once this branch has been merged.

bassosimone and others added some commits Apr 4, 2016

Merge remote-tracking branch 'origin/master' into feature/ndt
Conflicts:
	include/measurement_kit/common/error_or.hpp
Merge branch 'master' into feature/ndt
Conflicts:
	include/measurement_kit/common/funcs.hpp
Merge remote-tracking branch 'origin/master' into feature/ndt
Conflicts:
	include/measurement_kit/net/error.hpp
Merge remote-tracking branch 'origin/master' into feature/ndt
Conflicts:
	include/measurement_kit/common/logger.hpp
	include/measurement_kit/common/net_test.hpp
	src/common/logger.cpp
Merge remote-tracking branch 'origin/master' into feature/ndt
Conflicts:
	include/measurement_kit/common/funcs.hpp
utils: change split to return a list
A list is more practical given that we are going to remove
from the front of the list inside of NDT code.
logger: granular verbosity level control
Initially implemented in the NDT branch, to be backported to master.
Show outdated Hide outdated src/ndt/run_impl.hpp
if (err) {
disconnect_and_callback(ctx, err);
return;
}

This comment has been minimized.

@hellais

hellais May 18, 2016

Contributor

Maybe it would be cleaner to make this into a macro.

@hellais

hellais May 18, 2016

Contributor

Maybe it would be cleaner to make this into a macro.

This comment has been minimized.

@bassosimone

bassosimone May 19, 2016

Member

Yeah! Implemented in cb884a7.

@bassosimone

bassosimone May 19, 2016

Member

Yeah! Implemented in cb884a7.

template <MK_MOCK_NAMESPACE(test_c2s, run), MK_MOCK_NAMESPACE(test_meta, run),
MK_MOCK_NAMESPACE(test_s2c, run)>
void run_tests_impl(Var<Context> ctx, Callback<Error> callback) {

This comment has been minimized.

@hellais

hellais May 18, 2016

Contributor

I am thinking that it may be useful for a user of the API to be able to run specific tests. My understanding of how this call works is that you look at the tests that are supported by the NDT server and run through them in the order by which they are returned from the server.

Wouldn't it be better instead to have the order, and the types of tests to run, be defined by a user of the NdtTest DSL?

Perhaps having a call to set the types of test to run.

@hellais

hellais May 18, 2016

Contributor

I am thinking that it may be useful for a user of the API to be able to run specific tests. My understanding of how this call works is that you look at the tests that are supported by the NDT server and run through them in the order by which they are returned from the server.

Wouldn't it be better instead to have the order, and the types of tests to run, be defined by a user of the NdtTest DSL?

Perhaps having a call to set the types of test to run.

This comment has been minimized.

@hellais

hellais May 18, 2016

Contributor

Edit:

I realise now that apparently

The tests MUST be performed in the order received from the Server

I guess then the above suggestion would be to instead of setting test_suite = TEST_STATUS | TEST_META | TEST_C2S | TEST_S2C make it possible to select also a subset of all tests.

I am thinking this could be particularly useful in the case where one wants to run a download test, but not an upload test or vice-versa.

@hellais

hellais May 18, 2016

Contributor

Edit:

I realise now that apparently

The tests MUST be performed in the order received from the Server

I guess then the above suggestion would be to instead of setting test_suite = TEST_STATUS | TEST_META | TEST_C2S | TEST_S2C make it possible to select also a subset of all tests.

I am thinking this could be particularly useful in the case where one wants to run a download test, but not an upload test or vice-versa.

This comment has been minimized.

@bassosimone

bassosimone May 19, 2016

Member

I agree. I have implemented the possibility to specify which test to run in d9bdd24.

@bassosimone

bassosimone May 19, 2016

Member

I agree. I have implemented the possibility to specify which test to run in d9bdd24.

Show outdated Hide outdated src/ndt/protocol_impl.hpp
case TEST_S2C:
func = test_s2c_run;
break;
}

This comment has been minimized.

@hellais

hellais May 18, 2016

Contributor

Maybe it's good to add checks for all the supported NDT tests TEST_MID, TEST_SFW (even if they are not yet implemented in NDT) and as suggested by the NDT design doc:

The Client MUST drop the connection when it receives unknown test id, because such situation means an error on the Server's part

@hellais

hellais May 18, 2016

Contributor

Maybe it's good to add checks for all the supported NDT tests TEST_MID, TEST_SFW (even if they are not yet implemented in NDT) and as suggested by the NDT design doc:

The Client MUST drop the connection when it receives unknown test id, because such situation means an error on the Server's part

This comment has been minimized.

@bassosimone

bassosimone May 19, 2016

Member

Refactored the code in 77ef083 to simplify the flow and clarify that, on invalid test id, the connection with the server is closed. (Better to see the diff without considering space changes, for clarity.)

@bassosimone

bassosimone May 19, 2016

Member

Refactored the code in 77ef083 to simplify the flow and clarify that, on invalid test id, the connection with the server is closed. (Better to see the diff without considering space changes, for clarity.)

This comment has been minimized.

@bassosimone

bassosimone May 19, 2016

Member

Then also added 878db1c because 77ef083 was broken.

@bassosimone

bassosimone May 19, 2016

Member

Then also added 878db1c because 77ef083 was broken.

Show outdated Hide outdated src/ndt/test_c2s_impl.hpp
double now = time_now();
if (now - *previous > 0.5) {
double x =
(*count * 8) / 1000 / (now - *previous);

This comment has been minimized.

@hellais

hellais May 18, 2016

Contributor

Count is the result of calling size() on string, which is the size of the string in bytes so there is no need to multiply this by 8.

@hellais

hellais May 18, 2016

Contributor

Count is the result of calling size() on string, which is the size of the string in bytes so there is no need to multiply this by 8.

This comment has been minimized.

@hellais

hellais May 18, 2016

Contributor

On second thought, I guess it depends what is the unit of measure you have below.

If below you mean to use kilobits, then it's OK like this, but I would probably change the reading of the info log call to make the clearer, such as (Speed: %.2f kbit/s).

@hellais

hellais May 18, 2016

Contributor

On second thought, I guess it depends what is the unit of measure you have below.

If below you mean to use kilobits, then it's OK like this, but I would probably change the reading of the info log call to make the clearer, such as (Speed: %.2f kbit/s).

This comment has been minimized.

@bassosimone

bassosimone May 19, 2016

Member

I agree! Now using kbit/s, see e65cc9b!

@bassosimone

bassosimone May 19, 2016

Member

I agree! Now using kbit/s, see e65cc9b!

bassosimone added some commits May 19, 2016

src/ndt/run_impl.hpp: use macro for readability
Suggested by @hellais.

While there commit updated `.gitignore`.
ndt: say explicitly kbit/s rather than kb/s
As suggested by @hellais, kbit/s is more easily readable and less
ambiguous than kb/s, when the unit of speed is bit/s.
protocol_impl.hpp: refactor test selection code
Refactor code selecting the test to run and eventually erroring out
such that it's easier to see what happens if a test id is not recognized.

Specifically, calling the callback of the stage with an error should
lead to the connection being closed by the client.

This diff is best read enabling the `-w` option to ignore space changes.
Purely cosmetic change
This should make a diff that is listed in the conversation on github
obsolete, thus facilitating code review.
@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone May 19, 2016

Member

@hellais Thanks for all the comments! I think I have implemented all the recommended suggestions!

Member

bassosimone commented May 19, 2016

@hellais Thanks for all the comments! I think I have implemented all the recommended suggestions!

@hellais

This comment has been minimized.

Show comment
Hide comment
@hellais

hellais May 23, 2016

Contributor

Ok I have finished another pass of reviewing and testing of this branch and it looks good to me.

I think we can merge this into master!

🎉 👍 💯

Contributor

hellais commented May 23, 2016

Ok I have finished another pass of reviewing and testing of this branch and it looks good to me.

I think we can merge this into master!

🎉 👍 💯

@hellais

This comment has been minimized.

Show comment
Hide comment
@hellais

hellais May 23, 2016

Contributor

As discussed with @bassosimone adding support for obtaining the result of a NDT test will be added in a PR on top of this one.

Contributor

hellais commented May 23, 2016

As discussed with @bassosimone adding support for obtaining the result of a NDT test will be added in a PR on top of this one.

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone May 23, 2016

Member

The build failed because a header needs to be changed to reflect #627. Apparently github is currently stuck; I've pushed the fix and a couple of reverts to trigger it, but the web interface (and their backend) has not updated yet. Will merge once the build resumes and is fixed.

Member

bassosimone commented May 23, 2016

The build failed because a header needs to be changed to reflect #627. Apparently github is currently stuck; I've pushed the fix and a couple of reverts to trigger it, but the web interface (and their backend) has not updated yet. Will merge once the build resumes and is fixed.

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone May 23, 2016

Member

Thanks for the review, @hellais! Now merging!

Member

bassosimone commented May 23, 2016

Thanks for the review, @hellais! Now merging!

@bassosimone bassosimone merged commit c40e453 into master May 23, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-1.6%) to 90.271%
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bassosimone bassosimone deleted the feature/ndt branch May 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment