Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[22321] Benchmark example updated #5460

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[22321] Benchmark example updated #5460

wants to merge 9 commits into from

Conversation

Javgilavi
Copy link
Contributor

@Javgilavi Javgilavi commented Dec 4, 2024 •
edited by Mario-DL
Loading

Description

I remake the benchmark example to new example style maintaining the original functionality.

@Mergifyio backport 3.1.x 3.0.x

Fixes #5398

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • N/A Any new/modified methods have been properly documented using Doxygen.
  • N/A Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • N/A New feature has been documented/Current behavior is correctly described in the documentation.
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@Javgilavi Javgilavi added this to the v3.2.0 milestone Dec 4, 2024
@Javgilavi Javgilavi requested a review from Mario-DL December 9, 2024 06:21
@github-actions github-actions bot added the ci-pending PR which CI is running label Dec 9, 2024
rsanchez15
rsanchez15 requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@rsanchez15 rsanchez15 left a comment

Choose a reason for hiding this comment

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

Pending example tests

@Javgilavi Javgilavi force-pushed the feature/benchmark-example branch from 4c8a8db to 54b8494 Compare December 11, 2024 08:48
@rsanchez15 rsanchez15 requested review from Mario-DL and removed request for Mario-DL December 11, 2024 09:58
@Javgilavi Javgilavi requested review from Mario-DL and removed request for Mario-DL December 11, 2024 12:40
Mario-DL
Mario-DL requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Good first iteration overall. The PR brings an interesting example.
In addition to my comments below, we would need to add this PR content Refactor benchmark example to the versions.md and replace the N/A with a tick in the checklist.

examples/cpp/benchmark/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/benchmark/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/benchmark/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/benchmark/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/benchmark/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/benchmark/SubscriberApp.cpp Outdated Show resolved Hide resolved
examples/cpp/benchmark/README.md Outdated Show resolved Hide resolved
examples/cpp/benchmark/README.md Outdated Show resolved Hide resolved
examples/cpp/benchmark/README.md Outdated Show resolved Hide resolved
test/examples/test_benchmark.py Outdated Show resolved Hide resolved
@Javgilavi Javgilavi force-pushed the feature/benchmark-example branch from b1e7b77 to 791e055 Compare December 16, 2024 06:28
@Javgilavi Javgilavi requested review from Mario-DL and rsanchez15 and removed request for Mario-DL December 16, 2024 06:28
@Javgilavi
Refs #22321: Benchmark example updated
00eac34 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi
Refs #22321: Test CI for benchmark example and issue fixing
734d0b7 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi
Refs #22321: Fix type variable error
ad70617 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi
Refs #22321: Corrections after review
f5f419b 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi
Refs #22321: Uncrustify fix
444329e 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi
Refs #22321: Uncrustified 2
8fa9eec 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi
Refs #22321: Delate unused variable <sent> from PublisherApp.hpp
f51d7b1 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi Javgilavi force-pushed the feature/benchmark-example branch from cc2b477 to f51d7b1 Compare December 16, 2024 08:56
@Javgilavi Javgilavi requested review from Mario-DL and removed request for Mario-DL December 16, 2024 08:57
Mario-DL
Mario-DL requested changes Dec 16, 2024
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Leaving just some comments on previous suggestions

@Mario-DL
Copy link
Member

Mario-DL commented Dec 16, 2024 •
edited
Loading

Please, don't forget updating the versions.md and marking it in the contributor checklist

@Javgilavi
Refs #22321: Extra corrections and fixes
6fc1e8f 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi Javgilavi requested a review from Mario-DL December 16, 2024 11:04
Mario-DL
Mario-DL requested changes Dec 16, 2024
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Just couple of comments

test/examples/benchmark.compose.yml Outdated Show resolved Hide resolved
test/examples/benchmark.compose.yml Outdated Show resolved Hide resolved
@Javgilavi Javgilavi requested a review from Mario-DL December 16, 2024 14:57
@Javgilavi
Refs #22321: Compose simplify for test
2a4715b 
Signed-off-by: Javier Gil Aviles <javiergil@eprosima.com>
@Javgilavi Javgilavi force-pushed the feature/benchmark-example branch from 8fcd7b4 to 2a4715b Compare December 16, 2024 15:15
@Javgilavi Javgilavi requested review from Mario-DL and removed request for Mario-DL December 16, 2024 15:16
Mario-DL
Mario-DL approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

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

@Mario-DL Mario-DL Mario-DL approved these changes

@rsanchez15 rsanchez15 rsanchez15 approved these changes

Assignees
No one assigned
Labels
ci-pending PR which CI is running
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.

no benchmark example? and incorrect doc instructions
3 participants
@Javgilavi @Mario-DL @rsanchez15