Skip to content

Fix migration rule#10

Closed
efernandez wants to merge 13 commits intokinetic-edgefrom
CORE-6641_fix_migration_rule
Closed

Fix migration rule#10
efernandez wants to merge 13 commits intokinetic-edgefrom
CORE-6641_fix_migration_rule

Conversation

@efernandez
Copy link
Copy Markdown
Member

@efernandez efernandez commented Feb 22, 2017

Description of the buggy corner case not handled on the rosbag migration tool

Consider the following scenario for a message G with the following versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3

where G3 is the current msg, and G2 is the bag message that needs migration.

We need to check for a migration rule like this first:

G2 --> G3

instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the lazy check to mark the rules as valid in the make_update_rule (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123) method makes that rule pass initially, but later it fails because there's no rule for the sub fields.

This has been adapted from the commit ec8ec37 description.

Improvement to the validation check for the rules

The other commit 97fdf48 simply allows to improve the lazy check in make_update_rule and therefore allows to find a better placement/position for the message to be migrated inside of the chain rules. This helps to avoid marking rules as valid, that later would fail because there's no rule for the sub fields. The more precise check that fails later happens at the sub fields level, and there's already a comment on the code that suggest that the way this is handled should be improved (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L307).

FYI @servos @guillaumeautran

FYI @mikepurvis (Can you confirm I'm correctly targeting kinetic-edge here?)

deng02 and others added 9 commits February 8, 2017 21:37
We need more connection leve diagnostics in our logs but turning
on DEBUG for roscpp_internal generates a flood of information. We
only need visibility into when tcpros connections are created/dropped
for publishers and subscribers. The rospy nodes currently do this
but the roscpp ones do not.

A DEBUG macro for logging messages to the named logger
roscpp_internal.connections has been added.  This will
allow it to inherit the roscpp_internal log level or be
turned on separately to just get connection-related messages.
Add logger roscpp_internal.connections.
* Implemented rate-control-topic and rate-control-max-delay.

* Made rate control comparisons compare to time publisher rather than wall time.

* Added a check and a warning if the header is not the first in the message,

and removed an unnecessary import.

* Some quick formatting changes to simplify the final pull reuqest.

* Why is there little whitespace changes.

* Tabs to spaces, and other whitespace fixes.

* This is the last whitespace fix I swear.

* Removed unused function parameters.

* Fixed a bug preventing code from building after commit ecfb85f.

* Aligned print messages for rosbag PAUSED/DELAYED/RUNNING
* add test code for multibyte string

* merge Python 2 and 3 code paths
… confusing.

For example, [this warning msg](https://travis-ci.org/wg-perception/people/jobs/202019288#L3737) (in this [PR](wg-perception/people#49)):

 ```
 * WARN: unrecognized 'group' tag in <node> tag. Node xml is <test name="$(arg testnode_name)" pkg="rostest" test-name="hztest_test" type="hztest">
    <group unless="$(arg expected_success)">
      <node args="-r 0.5 $(find face_detector)/test/face_detector_noface_test_diamondback.bag" name="play" pkg="rosbag" type="play"/>
      <param name="hz" value="0.0"/>
    </group>
```

This is confusing because the tag in question is actually `<test>`, not `<node>`. Although the warning message refers to the exact <test> tag, I didn't try to read it because I was told the issue is with <node>, not <test>.

With this PR the message becomes a bit verbose but hope the readers get better idea.
@efernandez efernandez added the bug label Feb 22, 2017
@efernandez efernandez self-assigned this Feb 22, 2017
@efernandez efernandez requested a review from afakihcpr February 22, 2017 23:31
@mikepurvis
Copy link
Copy Markdown

Not quite. Please target the kinetic devel branch upstream.

mikepurvis and others added 4 commits February 23, 2017 09:49
fixes CORE-6641

Consider the following scenario for a message G with the following
versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3

where G3 is the CURRENT msg, and G2 is the BAG message that needs
migration.

We need to check for a migration rule like this first:

G2 --> G3

instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the 'lazy' check to valid the rules in the make_update_rule
method makes that rule pass initially, but later it fails because
there's no rule for the sub fields.
refs CORE-6641

Checking the sub rules are also valid we allow to check for different
positions in the chain for the BAG message we need to migrate, because
we don't really know where it should go, and the 'lazy' check for valid
in the make_update_rule method makes the suggested rules pass almost
always, so we can easily get a rule in the wrong place that later would
fail because there are no rules for the sub fields.
@efernandez efernandez force-pushed the CORE-6641_fix_migration_rule branch from 97fdf48 to 0c64db0 Compare February 23, 2017 14:49
@efernandez
Copy link
Copy Markdown
Member Author

OK, rebased on top of origin/kinetic-devel, and closing now to create a PR upstream.
Thanks.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants