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.

Sign up for GitHub

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

Jump to bottom

Fix throw in playback of split+compressed bagfiles #294

Merged
merged 5 commits into from
Mar 12, 2020
Merged

Fix throw in playback of split+compressed bagfiles #294

merged 5 commits into from
Mar 12, 2020

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Feb 13, 2020 •
edited by emersonknapp
Loading

Changes

  • Require compression_mode_ to not be none within load_next_file
  • Require decompressor_ to not be null if compression_mode_ is FILE in load_next_file

Closes #296

@zmichaels11 zmichaels11 marked this pull request as ready for review February 13, 2020 21:42
@zmichaels11
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/50bcd011e1f31e0477fe5c58c6f99c1a/raw/2d07178c55d483933adb583f2f4184765f0a4128/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launcher

@emersonknapp
Copy link
Collaborator

Is this a bugfix? If so, can we put in a regression test for it?

dabonnie
dabonnie suggested changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@dabonnie dabonnie left a comment

Choose a reason for hiding this comment

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

I'll be filing a bug shortly, but here's the current behavior:
ros2 bag play ... fails for multiple files, but is able to play the first file. This means that the method being changed, load_next_file, is not being used to load the first file. To simplify the code and improve unit tests, we should be loading all files in the same manner, i.e., one method.

rosbag2_compression/src/rosbag2_compression/sequential_compression_reader.cpp Outdated Show resolved Hide resolved
@dabonnie dabonnie mentioned this pull request Feb 14, 2020
Closed
@dabonnie
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container [Build Status]

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Feb 14, 2020 •
edited
Loading

A proper unit test for this regression will require a CompressionFactory so we can mock out ZstdDecompressor. I'll try and find some time to introduce that in a separate PR.

piraka9011
piraka9011 approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

LGTM

@piraka9011
Copy link
Contributor

A proper unit test for this regression will require a CompressionFactory so we can mock out ZstdDecompressor. I'll try and find some time to introduce that in a separate PR.

Open a ticket to track this please.

@zmichaels11 zmichaels11 mentioned this pull request Feb 14, 2020
Closed
@zmichaels11
Copy link
Contributor Author

@piraka9011 Opened #297

@prajakta-gokhale prajakta-gokhale self-assigned this Feb 24, 2020
@Karsten1987
Copy link
Collaborator

Karsten1987 commented Mar 10, 2020 •
edited
Loading

@zmichaels11 what's the status of this PR? Is this waiting on #297 ?

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Mar 10, 2020 •
edited
Loading

We want to put sufficient testing on the changes in this PR before merging it in.
@piraka9011 is going to merge in the work required to write the unit tests shortly.

Edit:
Relevant PR: #315

@zmichaels11 zmichaels11 mentioned this pull request Mar 11, 2020
Closed
@piraka9011 piraka9011 force-pushed the zmichaels11-patch-1 branch 2 times, most recently from 6c0cc40 to 89d3184 Compare March 11, 2020 23:52
zmichaels11 and others added 5 commits March 11, 2020 16:52
@zmichaels11 @piraka9011
Fix throw in playback of split+compressed bagfiles
38026a9 
* Require `compression_mode_` to not be none within `load_next_file`
* Require `decompressor_` to not be null if `compression_mode_` is `FILE` in `load_next_file`

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Fix PR

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@zmichaels11 @piraka9011
Added more context to error message.
d8c37af 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@zmichaels11 @piraka9011
Improve error messages in SequentialCompressionReader
c695cfe 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Add regression test for split/compression playback
f23184b 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Make test expectations stricter
3370b91 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
zmichaels11
zmichaels11 commented Mar 12, 2020
View reviewed changes
Copy link
Contributor Author

@zmichaels11 zmichaels11 left a comment

Choose a reason for hiding this comment

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

LGTM

Karsten1987
Karsten1987 approved these changes Mar 12, 2020
View reviewed changes
Copy link
Collaborator

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

However, can somebody please elaborate how this is fixing #296? It's not easily visible to me.

@piraka9011
Copy link
Contributor

piraka9011 commented Mar 12, 2020 •
edited by zmichaels11
Loading

However, can somebody please elaborate how this is fixing #296? It's not easily visible to me.

The main fix comes from this:

if (decompressor_ == nullptr) {
  throw std::runtime_error{
    "The bag file was not properly opened. "
    "Somehow the compression mode was set without opening a decompressor."
  };
}

Where the previous check was simply:

if (decompressor_) {
...
}

The previous if statement improperly throws when decompressor_ is not null when the intention was to throw when it is null.

@piraka9011
Copy link
Contributor

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

@zmichaels11 zmichaels11 merged commit b02fac7 into master Mar 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the zmichaels11-patch-1 branch March 12, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

@dabonnie dabonnie dabonnie approved these changes

Assignees

@piraka9011 piraka9011

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot play multiple compressed files
6 participants
@zmichaels11 @emersonknapp @dabonnie @piraka9011 @Karsten1987 @prajakta-gokhale