ros2 / rmw_connextdds

ROS 2 RMW layer for RTI Connext DDS Professional and RTI Connext DDS Micro.
Apache License 2.0
48 stars 33 forks source link

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

Closed trubio-rti closed 1 month ago

trubio-rti commented 5 months ago

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.

asorbini commented 5 months ago

Full CI:

sloretz commented 4 months ago

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 commented 1 month ago

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

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 commented 1 month ago
trubio-rti commented 1 month ago

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

asorbini commented 1 month ago

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

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

clalancette commented 1 month ago

Pulls: ros2/rmw_connextdds#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

clalancette commented 1 month ago
  • 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 commented 1 month ago

@clalancette A friendly ping to follow up

trubio-rti commented 1 month ago

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