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.

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

Add demos for using logger service #611

Merged

Conversation

Barry-Xu-2018
Copy link
Contributor

@Barry-Xu-2018 Barry-Xu-2018 commented Apr 18, 2023

Signed-off-by: Barry Xu <barry.xu@sony.com>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

overall looks good to me, some changes are requested.

demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
Signed-off-by: Barry Xu <barry.xu@sony.com>
Copy link
Contributor

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

some suggestions

demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
demo_nodes_py/demo_nodes_py/logging/use_logger_service.py Outdated Show resolved Hide resolved
demo_nodes_py/demo_nodes_py/logging/use_logger_service.py Outdated Show resolved Hide resolved
demo_nodes_py/demo_nodes_py/logging/use_logger_service.py Outdated Show resolved Hide resolved
demo_nodes_py/demo_nodes_py/logging/use_logger_service.py Outdated Show resolved Hide resolved
demo_nodes_py/demo_nodes_py/logging/use_logger_service.py Outdated Show resolved Hide resolved
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Contributor Author

I updated code according to your comments.
Please check again @iuhilnehc-ynos

@iuhilnehc-ynos
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

@clalancette CI green, can you do final review?

@clalancette clalancette self-assigned this May 4, 2023
demo_nodes_cpp/CMakeLists.txt Outdated Show resolved Hide resolved
demo_nodes_cpp/src/logging/use_logger_service.cpp Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
Output with default logger level:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noticing that when running the test here, the test itself takes like 1 second to run, but then it sits around for an additional 5 seconds before the test completes. That means each invocation of this test takes at least 6 seconds, and across 4 RMWs (rmw_fastrtps_cpp, rmw_fastrtps_dynamic_cpp, rmw_cyclonedds_cpp, and rmw_connextdds), this will add at least 24 seconds to our CI times.

Do you mind taking a look here and seeing what is causing that extra 5 second delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not only did this test take an extra 5 seconds, but I also noticed that all the other tests took at least 5 seconds as well. So, the issue is likely within the testing framework itself.
Currently, I am not familiar with the current testing framework, so it may take some time to investigate

Copy link
Contributor Author

@Barry-Xu-2018 Barry-Xu-2018 May 5, 2023

Choose a reason for hiding this comment

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

@clalancette

Find the below code to sleep 5 seconds in test framework. I think we can reduce the sleep time.

# TODO(hidmic): either make the underlying executables resilient to
# interruptions close/during shutdown OR adapt the testing suite to
# better cope with it.
import time
time.sleep(5)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@clalancette this is not really related to this PR, can we have another issue to follow-up? or if you have suggestions, can you share it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@clalancette this is not really related to this PR, can we have another issue to follow-up? or if you have suggestions, can you share it here?

Yes, you are correct. If you could please open a separate issue to reduce this timeout in the framework, then I think we are fine to move forward here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you could please open a separate issue to reduce this timeout in the framework, then I think we are fine to move forward here.

I created a new issue #628 to trace this problem. @clalancette

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Yadunund Yadunund added this to In progress in Iron Irwini via automation May 9, 2023
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One very minor thing to fix, then we'll be good to run another CI on this and merge.

demo_nodes_py/setup.py Outdated Show resolved Hide resolved
Signed-off-by: Barry Xu <barry.xu@sony.com>
@clalancette
Copy link
Contributor

clalancette commented May 16, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 207e22d into ros2:rolling May 16, 2023
2 checks passed
Iron Irwini automation moved this from In progress to Done May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants