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

Merged
merged 9 commits into from
Dec 17, 2024
Merged

[22321] Benchmark example updated #5460

merged 9 commits into from
Dec 17, 2024

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 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 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 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

@Mario-DL Mario-DL added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Dec 17, 2024
@Javgilavi Javgilavi merged commit e99d6f3 into master Dec 17, 2024
17 checks passed
@Javgilavi Javgilavi deleted the feature/benchmark-example branch December 17, 2024 07:45
@Javgilavi
Copy link
Contributor Author

@Mergifyio backport 3.1.x 3.0.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 17, 2024 •
edited
Loading

backport 3.1.x 3.0.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Dec 17, 2024
@Javgilavi @mergify
Benchmark example updated (#5460)
2bcbcc8 
* Refs #22321: Benchmark example updated

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Test CI for benchmark example and issue fixing

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Fix type variable error

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Corrections after review

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Uncrustify fix

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Uncrustified 2

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Delate unused variable <sent> from PublisherApp.hpp

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Extra corrections and fixes

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Compose simplify for test

Signed-off-by: Javier Gil Aviles <[email protected]>

---------

Signed-off-by: Javier Gil Aviles <[email protected]>
(cherry picked from commit e99d6f3)

# Conflicts:
#	versions.md
@mergify mergify bot mentioned this pull request Dec 17, 2024
Open
13 tasks
mergify bot pushed a commit that referenced this pull request Dec 17, 2024
@Javgilavi @mergify
Benchmark example updated (#5460)
27731c8 
* Refs #22321: Benchmark example updated

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Test CI for benchmark example and issue fixing

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Fix type variable error

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Corrections after review

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Uncrustify fix

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Uncrustified 2

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Delate unused variable <sent> from PublisherApp.hpp

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Extra corrections and fixes

Signed-off-by: Javier Gil Aviles <[email protected]>

* Refs #22321: Compose simplify for test

Signed-off-by: Javier Gil Aviles <[email protected]>

---------

Signed-off-by: Javier Gil Aviles <[email protected]>
(cherry picked from commit e99d6f3)
@mergify mergify bot mentioned this pull request Dec 17, 2024
Open
13 tasks
@Javgilavi Javgilavi restored the feature/benchmark-example branch December 17, 2024 10:17
@Javgilavi Javgilavi deleted the feature/benchmark-example branch December 17, 2024 11:58
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
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
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