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

if cache data exists, it needs to flush the data into the storage before shutdown #541

Merged
merged 3 commits into from
Oct 20, 2020
Merged

if cache data exists, it needs to flush the data into the storage before shutdown #541

merged 3 commits into from
Oct 20, 2020

Conversation

fujitatomoya
Copy link
Contributor

close #540

Signed-off-by: Tomoya.Fujita Tomoya.Fujita@sony.com

@fujitatomoya
if cache data exists, it needs to flush the data into the storage bef…
b54d184 
…ore shutdown.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya requested a review from a team as a code owner October 17, 2020 03:23
@fujitatomoya fujitatomoya requested review from jaisontj and dabonnie and removed request for a team October 17, 2020 03:23
@fujitatomoya
Copy link
Contributor Author

fujitatomoya commented Oct 17, 2020 •
edited
Loading

  • test report
# colcon test --packages-select ros2bag rosbag2 rosbag2_compression rosbag2_converter_default_plugins rosbag2_cpp rosbag2_performance_writer_benchmarking rosbag2_py rosbag2_storage rosbag2_storage_default_plugins rosbag2_test_common rosbag2_tests rosbag2_transport

-> Summary: 1269 tests, 0 errors, 0 failures, 8 skipped

#540 (comment) can be fixed with this PR.

@fujitatomoya fujitatomoya mentioned this pull request Oct 17, 2020
Closed
@fujitatomoya
cache_ needs to reset after writing to storage.
3b5ec61 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
jaisontj
jaisontj suggested changes Oct 19, 2020
View reviewed changes
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated
// if cache data exists, it must flush the data into the storage
if (!cache_.empty()) {
storage_->write(cache_);
// reset cache
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this code is used as is in multiple places now, can be put this inside a helper to avoid duplication? maybe a reset_cache method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, will do that.

jaisontj
jaisontj suggested changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@jaisontj jaisontj left a comment

Choose a reason for hiding this comment

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

Just one minor thing otherwise LGTM

dabonnie
dabonnie approved these changes Oct 19, 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 with the nit to de-duplicate the logic also on line 254.

@fujitatomoya
add reset_cache() method.
05c05d4 
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Contributor Author

@jaisontj @dabonnie

could you kindly check again?

dabonnie
dabonnie approved these changes Oct 20, 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, I'll kick off CI and let's merge if green.

@dabonnie
Copy link
Contributor

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

jaisontj
jaisontj approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@jaisontj jaisontj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM :)

Thank you @fujitatomoya

@jaisontj jaisontj merged commit 1daf833 into ros2:master Oct 20, 2020
@fujitatomoya
Copy link
Contributor Author

@jaisontj @dabonnie

thanks for the quick response!

adamdbrw pushed a commit that referenced this pull request Nov 2, 2020
@fujitatomoya @adamdbrw
if cache data exists, it needs to flush the data into the storage bef…
1b3c71a 
…ore shutdown (#541)

fix #540
* if cache data exists, it needs to flush the data into the storage before shutdown.
* cache_ needs to reset after writing to storage.
* add reset_cache() method.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
adamdbrw pushed a commit that referenced this pull request Nov 2, 2020
@fujitatomoya @adamdbrw
if cache data exists, it needs to flush the data into the storage bef…
09ad5ea 
…ore shutdown (#541)

fix #540
* if cache data exists, it needs to flush the data into the storage before shutdown.
* cache_ needs to reset after writing to storage.
* add reset_cache() method.

Signed-off-by: tomoya <Tomoya.Fujita@sony.com>
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
@fujitatomoya
if cache data exists, it needs to flush the data into the storage bef…
461b1ae 
…ore shutdown (#541)

fix #540 
* if cache data exists, it needs to flush the data into the storage before shutdown.
* cache_ needs to reset after writing to storage.
* add reset_cache() method.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
@fujitatomoya
if cache data exists, it needs to flush the data into the storage bef…
dee8ca1 
…ore shutdown (#541)

fix #540
* if cache data exists, it needs to flush the data into the storage before shutdown.
* cache_ needs to reset after writing to storage.
* add reset_cache() method.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
skudryas pushed a commit to skudryas/rosbag2 that referenced this pull request Mar 12, 2021
@fujitatomoya
if cache data exists, it needs to flush the data into the storage bef…
dae0b5f 
…ore shutdown (ros2#541)

fix ros2#540
* if cache data exists, it needs to flush the data into the storage before shutdown.
* cache_ needs to reset after writing to storage.
* add reset_cache() method.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos left review comments

@jaisontj jaisontj jaisontj approved these changes

@dabonnie dabonnie dabonnie approved these changes

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

Successfully merging this pull request may close these issues.

ros2 bag play fails with Opened database for READ_ONLY
4 participants
@fujitatomoya @dabonnie @jaisontj @iuhilnehc-ynos