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

Fix/split by time #1022

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Fix/split by time #1022

merged 1 commit into from
Jun 17, 2022

Conversation

Kaju-Bubanja
Copy link
Contributor

Fix #966

@Kaju-Bubanja Kaju-Bubanja requested a review from a team as a code owner May 30, 2022 12:27
@Kaju-Bubanja Kaju-Bubanja requested review from emersonknapp and jhdcs and removed request for a team May 30, 2022 12:27
@jhdcs
Copy link
Contributor

jhdcs commented Jun 7, 2022

Thank you for your submission! However, there appear to be some linting and build errors. Could you take a quick look at them and see what's going on?

@Kaju-Bubanja
Copy link
Contributor Author

Sorry I tried to look at the output of the job, but it's just too much to look at. Also it seems totally unrelated to the code changes I did and more about the setup of the job. For example the first error I find when searching for error is:

  dpkg: error processing archive /var/cache/apt/archives/rti-connext-dds-6.0.1_6.0.1.25-1_amd64.deb (--unpack):
   new rti-connext-dds-6.0.1 package pre-installation script subprocess returned error exit status 10
  Errors were encountered while processing:
   /var/cache/apt/archives/rti-connext-dds-6.0.1_6.0.1.25-1_amd64.deb

which are totally unrelated to the changes I made. There are also a bunch of other errors and somehow there is no option to scroll to the bottom of the log? I was scrolling for a good few minutes until I reached line 110000 or so, but still wasn't at the end. So I would be happy if somebody could have a look at why the the job is failing or maybe point me to some resources how to diagnose better what the problem is. Because having just one huge scrollable thousands of lines output file to go trough is unpractical. Is there a way to make the job stop on the first error it encounters, like colcon works in the terminal? Or is there a way to jump to the end of the output?

1 similar comment
@Kaju-Bubanja
Copy link
Contributor Author

Sorry I tried to look at the output of the job, but it's just too much to look at. Also it seems totally unrelated to the code changes I did and more about the setup of the job. For example the first error I find when searching for error is:

  dpkg: error processing archive /var/cache/apt/archives/rti-connext-dds-6.0.1_6.0.1.25-1_amd64.deb (--unpack):
   new rti-connext-dds-6.0.1 package pre-installation script subprocess returned error exit status 10
  Errors were encountered while processing:
   /var/cache/apt/archives/rti-connext-dds-6.0.1_6.0.1.25-1_amd64.deb

which are totally unrelated to the changes I made. There are also a bunch of other errors and somehow there is no option to scroll to the bottom of the log? I was scrolling for a good few minutes until I reached line 110000 or so, but still wasn't at the end. So I would be happy if somebody could have a look at why the the job is failing or maybe point me to some resources how to diagnose better what the problem is. Because having just one huge scrollable thousands of lines output file to go trough is unpractical. Is there a way to make the job stop on the first error it encounters, like colcon works in the terminal? Or is there a way to jump to the end of the output?

@Kaju-Bubanja Kaju-Bubanja force-pushed the fix/split_by_time branch 2 times, most recently from 6a29594 to 639ebad Compare June 14, 2022 10:56
@jhdcs
Copy link
Contributor

jhdcs commented Jun 14, 2022

If you click "Details" on the failing test (Rpr__rosbag2__ubuntu_jammy_amd64), you will be taken to the landing page for Jenkins. On the left side of the screen, look for a button that says "Console Output" and click on it.

On the page that you will arrive to, look on the left side of the screen again. Just below the "Previous Build" button, there will be a list of links labeled "Console Sections". You will want to click on each that says stderr.

The zstd_vendor(stderr) I don't think you can do anything about. However, the section with rosbag2_cpp(stderr) is highlighting a couple issues related to the should_split_bagfile function that are preventing it from building.

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.

@Kaju-Bubanja Thanks for your PR.
Overall looks good among a nitpick with uncrustify linter error.
I also found one more bug related to the bag split during review.
We are not initializing file_info.starting_time to maximum in void SequentialWriter::split_bagfile().
And further on we will try to compare it with current message timestamp in line 311 of write method

According to the https://www.enseignement.polytechnique.fr/informatique/INF478/docs/Cpp/en/cpp/chrono/time_point/time_point.html default constructor for time_point, creates a time_point with a value of Clock's epoch. Which is going to be always less than time stamp from current message.

Could you please add initialization

file_info.starting_time =
    std::chrono::time_point<std::chrono::high_resolution_clock>(std::chrono::nanoseconds::max());

in https://github.com/ros2/rosbag2/pull/1022/files#diff-7ab16bf3d261a150307f030f5051570db9d67316ec76de8b1a199f592f947b09R273-R274

@Kaju-Bubanja Kaju-Bubanja force-pushed the fix/split_by_time branch 2 times, most recently from 32fd513 to 6a279ad Compare June 15, 2022 08:22
@Kaju-Bubanja
Copy link
Contributor Author

Thanks for the review, should be all addressed now. Currently 2 tests are failing, which seem unrelated to the PR: https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/118/testReport/ Not sure what the problem is with these tests

@MichaelOrlov
Copy link
Contributor

@Kaju-Bubanja

Thanks for the review, should be all addressed now. Currently 2 tests are failing, which seem unrelated to the PR: https://build.ros2.org/job/Rpr__rosbag2__ubuntu_jammy_amd64/118/testReport/ Not sure what the problem is with these tests

I confirm that failing test_play_services__rmw_fastrtps_cpp is unrelated and known failure.

style: fix diff

style: remove additional spaces

style: typo

fix: added missing input parameter

refactor: make variable private

refactor: fix linting error

fix: Add missing initialization

style: linter

refactor: invert logic

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>
@MichaelOrlov
Copy link
Contributor

Running CI:
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10428/

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

@MichaelOrlov MichaelOrlov merged commit ad484ba into ros2:master Jun 17, 2022
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jun 17, 2022

@Kaju-Bubanja Thanks for your PR one more time.

hrfuller pushed a commit to hrfuller/rosbag2 that referenced this pull request Aug 23, 2023
style: fix diff

style: remove additional spaces

style: typo

fix: added missing input parameter

refactor: make variable private

refactor: fix linting error

fix: Add missing initialization

style: linter

refactor: invert logic

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>
fujitatomoya added a commit to fujitatomoya/rosbag2 that referenced this pull request Apr 17, 2024
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
MichaelOrlov pushed a commit that referenced this pull request Apr 24, 2024
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
emersonknapp pushed a commit that referenced this pull request May 14, 2024
* pipe in --max-bag-splits arg down to storage options

* remove old bags after max splits reached

* fix double exit

* fix: splitting by time feature (#1022)

style: fix diff

style: remove additional spaces

style: typo

fix: added missing input parameter

refactor: make variable private

refactor: fix linting error

fix: Add missing initialization

style: linter

refactor: invert logic

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>

* write tests for max bagile splits, and adjust test helper signatures

* use now

* Don't use system time when updating duration. Respond to other feedback

* move max bag splits checking to cpp, add pybind. fix lint

* fix test and lint

* lint line length

* Revert "fix double exit"

---------

Signed-off-by: Kaju Bubanja <bubanja.kaju@gmail.com>
Co-authored-by: Kaju-Bubanja <bubanja.kaju@gmail.com>
emersonknapp added a commit that referenced this pull request Jun 10, 2024
* Link and compile against rosbag2_storage_mcap: Fixed issue 1492 (#1496) (#1498)

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 7fcb703)

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* [humble] Bugfix for incorrect playback rate changes when pressing buttons (backport #1513) (#1515)

* Bugfix for incorrect playback rate changes when pressing buttons (#1513)

- Playback rate expected to be changed by 10% with each
increase/decrease step.
- Use +0.1 and -0.1 in decrease/increase rate formula instead of
multiply by factor of the 1.1 and 0.9 respectively.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 95f78b6)

# Conflicts:
#	rosbag2_transport/src/rosbag2_transport/player.cpp

* Address merge conflicts after auto-backporting

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* call cv.wait_until only if necessary. (#1521) (#1523)

* call cv.wait_until only if necessary.

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

* add comment to avoid extra delay for performance.

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

---------

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit a16704b)

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* [humble] Install signal handlers in recorder only inside record method (backport #1464) (#1526)

* Install signal handlers in recorder only inside record method (#1464)

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 195e406)

# Conflicts:
#	rosbag2_py/src/rosbag2_py/_transport.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] `Recording stopped` prints only once. (backport #1530) (#1535)

* `Recording stopped` prints only once. (#1530)

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 73b0772)

# Conflicts:
#	rosbag2_transport/src/rosbag2_transport/recorder.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] Give proper log message for `--start-paused` (backport #1537) (#1541)

* Add proper message for --start-paused (#1537)

Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
(cherry picked from commit 317286c)

# Conflicts:
#	rosbag2_transport/src/rosbag2_transport/recorder.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* 0.15.9 (#1551)

* 0.15.9

Signed-off-by: Audrow Nash <audrow@intrinsic.ai>

* Update rosbag2_transport/CHANGELOG.rst

Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>

---------

Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] Add default initialization for CompressionOptions (backport #1539) (#1546)

* Add default initialization for CompressionOptions (#1539)

* feat: add sane defaults for CompressionOptions

Signed-off-by: Arne Böckmann <a.boeckmann@cellumation.com>

* Update rosbag2_compression/include/rosbag2_compression/compression_options.hpp

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Arne Böckmann <a.boeckmann@cellumation.com>

---------

Signed-off-by: Arne Böckmann <a.boeckmann@cellumation.com>
Co-authored-by: Arne Böckmann <a.boeckmann@cellumation.com>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 931bf54)

# Conflicts:
#	rosbag2_compression/include/rosbag2_compression/compression_options.hpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Arne B <arne@rnae.de>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* Fix/zstd vendor does not find system zstd (#1111) (#1560)

cmake did not find the Findzstd.cmake in cmake/Modules

Since we cannot use pkg-config for some Windows issues, the parsing of
the version is done by looking for the string in zstd.h.

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
(cherry picked from commit e7e7269)

Co-authored-by: DasRoteSkelett <matthias.schoepfer@googlemail.com>

* [humble] Use rw_lock to protect mcap metadata lists. (backport #1561) (#1567)

* Use rw_lock to protect mcap metadata lists. (#1561)

* use rw_lock to protect mcap metadata lists.

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

* introduce MCAPStorage::write_lock_free private method.

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

---------

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 90d1da8)

# Conflicts:
#	rosbag2_storage_mcap/src/mcap_storage.cpp

* Resolve merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* Add /bigobj to MSVC compiles. (#1571)

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

* Fix split by time. (backport #1022) (#1616)

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

* [humble] Add BagSplitInfo service call on bag close (backport #1422) (#1637)

* Add BagSplitInfo service call on bag close (#1422)

- Note: The `BagSplitInfo::opened_file` will have empty string to
indicate that it was "bag close" and not bag split event.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit ba199d0)

# Conflicts:
#	rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp

* Fix merge conflicts

- Ensure that writer_ is destructed before intercepted fake_metadata_

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [Humble] Resolve recording option problem (backport #1649) (#1651)

* Resolve recording option problem (#1649)

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 4914ab3)

# Conflicts:
#	ros2bag/ros2bag/verb/record.py

* Fix cherry-pick conflicts for mergify/bp/humble/pr-1649 (#1652)

Signed-off-by: Barry Xu <barry.xu@sony.com>

---------

Signed-off-by: Barry Xu <barry.xu@sony.com>
Co-authored-by: Barry Xu <barry.xu@sony.com>

* [humble] Add --log-level to ros2 bag play and record (#1655)

* Add --log-level to ros2 bag play and record

Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Fix missing import

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

---------

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] Bugfix for writer not being able to open again after closing (backport #1599) (#1653)

* [iron] Bugfix for writer not being able to open again after closing (backport #1599) (#1635)

* re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too.

Signed-off-by: Yannick Schulz <yschulz854@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer

Signed-off-by: Yannick Schulz <yschulz854@gmail.com>

* Remove unrelated delta

- message_definitions_ was intentionally allocated on the stack and
should persist between writer close() and open() because it represents
cache for message definitions which is not changes.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Don't call virtual methods from destructors

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Cleanup 'rosbag2_directory_next' after the test run

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Protect Writer::open(..) and Writer::close() with mutex on upper level

- Rationale: To avoid race conditions if open(..) and close() could be
ever be called from different threads.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Bugfix for WRITE_SPLIT callback not called for the last compressed file

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Bugfix for lost messages from cache when closing compression writer

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Address build failure by using rcpputils::fs instead of std::filesystem

- Note: On Iron we haven't migrated to the std::filesystem and using
rcpputils::fs

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Adopt failing 'open_succeeds_twice' test for Iron

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Return from writer's open() immediately if storage already open

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Yannick Schulz <yschulz854@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Yannick Schulz <yschulz854@gmail.com>
(cherry picked from commit a360d9b)

# Conflicts:
#	rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
#	rosbag2_cpp/src/rosbag2_cpp/writer.cpp
#	rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Fix for segfault in open_twice test

- Ensure that writer_ is destructed before intercepted fake_metadata_

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Fix for "open_succeeds_twice" test failure on second run

- Use std::filesystem for temp files and folders operation. For some
reason rcpputils::fs::delete_all(folder_name) wasn't able to delete temp
folder with subfolders.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Adopt changes in TestRosbag2CPPAPI::minimal_writer_example for humble

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test. This is not a bug - this is by design.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Audrow Nash <audrow@openrobotics.org>
Co-authored-by: Arne B <arne@rnae.de>
Co-authored-by: DasRoteSkelett <matthias.schoepfer@googlemail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Barry Xu <barry.xu@sony.com>
Co-authored-by: Roman <rsokolkov@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.

Playing of bag which is recorded with split by duration/size is not working properly .
3 participants