Skip to content

Conversation

@k-rister
Copy link
Member

No description provided.

@k-rister k-rister requested a review from atheurer July 30, 2018 18:40
@k-rister
Copy link
Member Author

Hmm, not sure what is going on with travis-ci but something seems off.

@ndokos
Copy link
Member

ndokos commented Jul 30, 2018

It's a unit test failure. Try this in your local branch:

  cd ...agent/bench-scripts
  ./unittests test-21

It should fail.

@k-rister
Copy link
Member Author

Thanks @ndokos. I looked at the output and I guess I just scrolled past that without seeing it (multiple times)...

@portante portante added this to the v0.51 milestone Jul 31, 2018
trex_use_l2="n"
benchmark_run_dir=""
traffic_directions="bidirec"
traffic_directions="bidirectional"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be binary compatible, and support bidirec to mean bidirectional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There are a lot more changes coming like this, we are going to be changing the entire option structure and don't have any need for backwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we make these changes in 0.52 instead of 0.51 so that you can accumulate a number of them into one release?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option structure changes are not pending real soon, that is more a forward looking viewpoint. The changes and additions in this PR are needed to deploy new functionality to users sooner rather than later.

@portante
Copy link
Member

portante commented Aug 3, 2018

Let's wait for @atheurer to review before we merge.

Do we have any unit tests for trafficgen?

- Use the same parameter and options as binary-search.py for better
  consistency.
- Lots of code rework since the trex-txrx-profile traffic generator is
  a bit different than previous traffic generators since most of the
  traffic parameters exist in the profile (on a per stream basis)
  instead of as CLI arguments (and applying to all streams).
… turned off

- It does not make sense to aggregate some data sets so add a flag,
  get_label('skip_aggregate_label'), which will force datasets to be
  ignored when aggregation is being performed.
@k-rister
Copy link
Member Author

k-rister commented Aug 3, 2018

This PR was rebased on top of #867 since that is a dependency.

@atheurer - I've added a first pass at postprocessing the TRex profiler data that is produced by the new trex-txrx-profile traffic generator. I think I've managed to follow your data model but would appreciate a review.

Copy link
Contributor

@atheurer atheurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me.

- This makes it easier to postprocess data since the requirements are
  lower.  This includes the ability to test the postprocessor on a
  system that does not run the benchmark.
@portante portante merged commit c7d3589 into distributed-system-analysis:master Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants