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

fix: "Failed to parse type hash" message was overly spammy [ros2-50] #149

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

trubio-rti
Copy link
Collaborator

Having this message printed with too high of a verbosity level makes it too spammy in normal executions of the RMW, making it hard to work with the log files.

@trubio-rti trubio-rti requested a review from asorbini May 7, 2024 17:20
@asorbini
Copy link
Collaborator

asorbini commented May 7, 2024

Full CI:

  • Linux: Build Status
  • Linux (aarch64): Build Status (stopped because not relevant)
  • Windows: Build Status

Copy link
Collaborator

@asorbini asorbini 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.

@sloretz
Copy link
Contributor

sloretz commented May 16, 2024

In 🧇 meeting we talked about taking this change and also adding a "log once". The log once call would say there was a type mismatch, and tell users to switch the logging mode to debug to see more info. Looking closer I now see that this is using an rmw_connext specific logging macro, and there's no log-once equivalent.

@clalancette what do you think about taking this change as-is?

@trubio-rti trubio-rti changed the title fix: "Failed to parse type hash" message was overly spammy (ros2-50) fix: "Failed to parse type hash" message was overly spammy [ros2-50] Jun 6, 2024
@trubio-rti
Copy link
Collaborator Author

The CI jobs have expired, I'm running them again:

  • Linux: Build Status
  • Windows: Build Status

Regarding @sloretz's comment, we can file a task to add the log-once feature but I prefer to keep it out of this PR to avoid feature creeping.

@trubio-rti
Copy link
Collaborator Author

  • Linux: Build Status
  • Windows: Build Status

@trubio-rti
Copy link
Collaborator Author

Linux build failed due to a lack of disk space in the CI machine. Re-running without changes: Build Status

@asorbini
Copy link
Collaborator

@clalancette We are ready to merge this PR, but we have a couple of questions:

  • Are "merge commits" ok or should @trubio-rti rebase the branch?

  • If merge commits are ok, do they need the "Signed-off" tag in the message?

Based on your answers we might have to update (and re-run CI as a formality) before merging. Thank you for your help!

@clalancette
Copy link
Contributor

Pulls: #149
Gist: https://gist.githubusercontent.com/clalancette/1c2039635efe8ba930f73d1f3b5dd0cf/raw/c1bec87478370bddfd63538fb5d76d04488bdb5e/ros2.repos
BUILD args: --packages-above-and-dependencies rmw_connextdds_common
TEST args: --packages-above rmw_connextdds_common
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14453

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

@clalancette
Copy link
Contributor

  • Are "merge commits" ok or should @trubio-rti rebase the branch?

No, we never use Merge commits in the final commit. That said, using the "Squash and merge" button in GitHub should always do the right thing, which is what we use everywhere else.

That said, it looks like CI on this change wasn't completely run, so I ran it again.

@MichaelOrlov
Copy link

@clalancette A friendly ping to follow up

@clalancette clalancette self-assigned this Aug 22, 2024
@trubio-rti
Copy link
Collaborator Author

CI is ok, as we don't run for ARM or RHEL

@trubio-rti trubio-rti merged commit 500baaa into rolling Aug 23, 2024
1 check passed
@clalancette clalancette deleted the feature/ros2-50 branch August 23, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants