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

Refactor Compression Reader/Writers to use the CompressionFactory #315

Merged
merged 2 commits into from
Mar 11, 2020
Merged

Refactor Compression Reader/Writers to use the CompressionFactory #315

merged 2 commits into from
Mar 11, 2020

Conversation

piraka9011
Copy link
Contributor

@piraka9011 piraka9011 commented Mar 10, 2020 •
edited
Loading

  • Refactored the compression setup in the readers/writers to use the CompressionFactory.
  • Added a MockCompression and MockCompressionFactory for future work.

Note: CompressionFactory methods were made virtual so that they could be mocked.

Closes #297

Signed-off-by: Anas Abou Allaban aabouallaban@pm.me

@piraka9011
Refactor reader and write to use compression factory
9c2fc58 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011 piraka9011 changed the title WIP: Refactor reader and write to use compression factory WIP: Refactor Compression Reader/Writers to use the CompressionFactory Mar 10, 2020
@piraka9011 piraka9011 changed the title WIP: Refactor Compression Reader/Writers to use the CompressionFactory Refactor Compression Reader/Writers to use the CompressionFactory Mar 10, 2020
@piraka9011 piraka9011 marked this pull request as ready for review March 10, 2020 04:26
dabonnie
dabonnie approved these changes Mar 10, 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.

LGTM

zmichaels11
zmichaels11 suggested changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

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

Can you add tests for checking that the mocked methods are called?

@piraka9011
Copy link
Contributor Author

Can you add tests for checking that the mocked methods are called?

I was planning to do that in another PR but I can also introduce it in this one.

@zmichaels11
Copy link
Contributor

I'd prefer them done in this PR since changes to public API were made.

@zmichaels11 zmichaels11 self-requested a review March 10, 2020 18:14
@piraka9011
Add tests to verify compression is called in reader/writers
f3ef417 
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/piraka9011/0416e21c428d6b62d652dce4dc905626/raw/f1e43bcfccd2e58b1d232526ba512e36badf2598/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launcher

@zmichaels11
Copy link
Contributor

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

@zmichaels11
Copy link
Contributor

It looks like CI for windows/linux if failing due to CI outage...

@zmichaels11
Copy link
Contributor

zmichaels11 commented Mar 10, 2020 •
edited
Loading

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

Edit: ci.ros2.org was still experiencing downtime, trying again

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

This was referenced Mar 10, 2020
Merged
Closed
@piraka9011
Copy link
Contributor Author

piraka9011 commented Mar 11, 2020 •
edited
Loading

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

Karsten1987
Karsten1987 approved these changes Mar 11, 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.

haven't looked at the changes in detail, but I look the approach of not relying on zstd

@piraka9011 piraka9011 merged commit 93864bc into ros2:master Mar 11, 2020
@zmichaels11 zmichaels11 deleted the refactor-compression-factory branch March 11, 2020 21:33
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

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@dabonnie dabonnie dabonnie approved these changes

@emersonknapp emersonknapp Awaiting requested review from emersonknapp

@prajakta-gokhale prajakta-gokhale Awaiting requested review from prajakta-gokhale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CompressionFactory so impl of Compressor/Decompressor can be changed
4 participants
@piraka9011 @zmichaels11 @Karsten1987 @dabonnie