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

set_read_order: return success #1177

Merged
merged 7 commits into from
Nov 30, 2022

Conversation

james-rms
Copy link
Contributor

@james-rms james-rms commented Nov 23, 2022

This PR makes these changes:

  1. Notes to client code that set_read_order should only be called after open(). There are cases where a storage plugin actually needs to read the underlying bag file in order to determine whether a given read order is supported. For example, if a timestamp index is required to read in timestamp order, the storage plugin needs to know whether that is present before accepting a request to read in timestamp order.
  2. Throw an exception from within SequentialReader if read order is set before opening the underlying storage.
  3. changes set_read_order to return a boolean for success or failure. This makes the flow for client code attempting to set a read order more ergonomic. Previously a client would have to check for a std::runtime_error and possibly inspect the exception text to ensure that the exception is about whether the read order is supported, rather than something unrelated.

@james-rms james-rms requested a review from a team as a code owner November 23, 2022 03:38
@james-rms james-rms requested review from emersonknapp and MichaelOrlov and removed request for a team November 23, 2022 03:38
@james-rms james-rms force-pushed the jrms/read-order-return-support branch 4 times, most recently from 74a7306 to a33b1b4 Compare November 23, 2022 04:38
@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/e5759b670fc0136412f5d739932ca559/raw/aea5e00d3f23132b22e229fe8ebed53517a0969e/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_storage rosbag2_storage_sqlite3 rosbag2_transport
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_storage rosbag2_storage_sqlite3 rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11172

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

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

I have a concern about not throwing exception in open if we fail to setup read order.

rosbag2_cpp/src/rosbag2_cpp/readers/sequential_reader.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

Thank you @james-rms. Now LGTM if CI will pass.

@james-rms
Copy link
Contributor Author

@MichaelOrlov I believe the failures in https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/431/ are not related to this PR - would you be able to kick off a full CI run on the build farm?

@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

Ok re-running full CI since Rpr passed
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11187

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

@MichaelOrlov
Copy link
Contributor

It seems Windows CI build is broken on build farm. I have the same issue in another PR #1175 (comment)
@clalancette Is it known issue?

@clalancette
Copy link
Contributor

@clalancette Is it known issue?

This time of year, the AWS spot prices fluctuate wildly, so our workers sometimes get unceremoniously yanked from underneath us. Just try again; it will eventually work.

Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/read-order-return-support branch from 6bbaf4e to 3a7c9d9 Compare November 29, 2022 20:29
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/read-order-return-support branch from b352ce0 to c2b36aa Compare November 30, 2022 01:01
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Nov 30, 2022

Re-running Windows build

  • Windows Build Status

@james-rms
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/james-rms/db3e068b715188676e7d53c6ee250379/raw/aea5e00d3f23132b22e229fe8ebed53517a0969e/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_tests
TEST args: --packages-above rosbag2_cpp rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11192

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

@james-rms james-rms merged commit 1accc18 into ros2:rolling Nov 30, 2022
@james-rms james-rms deleted the jrms/read-order-return-support branch November 30, 2022 08:56
ricardo-manriquez pushed a commit to ricardo-manriquez/rosbag2 that referenced this pull request Dec 7, 2022
Signed-off-by: James Smith <james@foxglove.dev>

Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
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.

4 participants