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

Add append mode to Mcap Writer in the Typescript Core package #1016

Closed
wants to merge 4 commits into from
Closed

Add append mode to Mcap Writer in the Typescript Core package #1016

wants to merge 4 commits into from

Conversation

alexern14
Copy link
Contributor

Public-Facing Changes

  • Added data end offset information to Typescipt Mcap Indexed Reader.
  • Added append mode to the Mcap Writer in the Typescript Mcap core package.

Description

  • Added data end offset information to Typescipt Mcap Indexed Reader for Mcap appending purposes.
  • Added append mode to the Mcap Writer in the Typescript Mcap core package in order to append metadata and attachments to existing Mcap files via the @mcap/core package.

@alexern14 alexern14 mentioned this pull request Nov 15, 2023
Closed
@alexern14 alexern14 changed the title Feature/mcap writer append mode Add append mode to Mcap Writer in the Typescript Core package Nov 15, 2023
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch from d2f966f to 073c194 Compare November 15, 2023 19:37
jtbandes
jtbandes reviewed Nov 16, 2023
View reviewed changes
Copy link
Member

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

This approach looks nicer. I think we could put the seek method in a separate interface to make the change backwards compatible.

FYI you will need to merge or rebase since I merged #1014 which updates some lint rules.

typescript/core/src/IWritable.ts Outdated Show resolved Hide resolved
typescript/core/src/McapIndexedReader.ts Outdated Show resolved Hide resolved
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch 4 times, most recently from 01fb93c to 9eb6a8a Compare November 16, 2023 17:30
@alexern14 alexern14 requested a review from jtbandes November 16, 2023 17:31
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch from 9eb6a8a to 0b93060 Compare November 16, 2023 17:36
jtbandes
jtbandes reviewed Nov 21, 2023
View reviewed changes
Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

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

Thanks for the continued work on this. In order to merge we will need to get some test coverage on this code. If you have some time & energy to put in, that would be appreciated – I think you should be able to follow some of the existing test patterns in McapWriter.test.ts, to create tests that write a file and then append to it with a 2nd writer instance, and demonstrating that the appended file has the correct records/indexes in it.

@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch from 987e51d to 7039375 Compare November 21, 2023 17:46
@alexern14 alexern14 requested a review from jtbandes November 21, 2023 17:49
jtbandes
jtbandes reviewed Nov 21, 2023
View reviewed changes
typescript/core/src/McapWriter.test.ts Show resolved Hide resolved
typescript/core/src/McapIndexedReader.ts Outdated Show resolved Hide resolved
typescript/core/src/McapIndexedReader.ts Outdated Show resolved Hide resolved
typescript/core/src/McapWriter.ts Outdated Show resolved Hide resolved
typescript/core/src/McapWriter.ts Outdated Show resolved Hide resolved
typescript/core/src/McapWriter.ts Outdated Show resolved Hide resolved
typescript/core/src/McapWriter.test.ts Show resolved Hide resolved
typescript/core/src/McapWriter.ts Show resolved Hide resolved
typescript/core/src/McapWriter.ts Outdated Show resolved Hide resolved
typescript/core/src/IAppendWritable.ts Outdated Show resolved Hide resolved
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch 2 times, most recently from 074de29 to e3c8f56 Compare November 22, 2023 18:32
@alexern14 alexern14 requested a review from jtbandes November 22, 2023 18:37
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch 4 times, most recently from 05384b8 to 4975788 Compare November 22, 2023 21:18
@alexern14
Copy link
Contributor Author

@jtbandes Thanks for the feedback! Addressed all your comments.

@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch 2 times, most recently from 3a8ebb9 to 4e58b74 Compare December 1, 2023 17:45
@jtbandes
Copy link
Member

jtbandes commented Dec 1, 2023

Sorry for the delay, this is on my radar but it might take a little while before I get more time to review it fully.

@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch from 4e58b74 to 1f2dbf9 Compare December 7, 2023 18:25
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch from 1f2dbf9 to 67f0d17 Compare December 19, 2023 21:55
@jtbandes jtbandes requested a review from achim-k December 19, 2023 23:28
@alexern14 alexern14 force-pushed the feature/mcap-writer-append-mode branch from 67f0d17 to a8fe009 Compare January 9, 2024 20:37
@jtbandes
Copy link
Member

Still planning on reviewing this but we've had a few other high-priority things come up. Thanks for your patience!

@jtbandes jtbandes mentioned this pull request Feb 13, 2024
Merged
@jtbandes
Copy link
Member

Made various updates & fixes; opened a new PR here: #1060

jtbandes added a commit that referenced this pull request Mar 12, 2024
@jtbandes @alexern14
typescript: add McapWriter.InitializeForAppending() (#1060)
d7c0bb3 
### Public-Facing Changes

- Added `InitializeForAppending()` to `McapWriter`.
- `McapIndexedReader` now exposes `dataEndOffset` and `dataSectionCrc`.

### Description

Append mode works by using `McapIndexedReader` to read the summary
section, loading all indexes, channels, etc. from the footer into
memory, then chopping off everything from DataEnd onwards and continuing
to write from there.

This does not work if the `DataEnd` record contains extra data or
padding. This seems to be a fundamental flaw with the spec; a separate
PR will be raised to change the spec to disallow this.

See also:
https://github.com/foxglove/mcap/blob/4d62967abe95e755b91f16cce390f6560d18e07e/go/cli/mcap/utils/mcap_amendment.go

Supersedes / closes #1016
Resolves FG-5821

---------

Co-authored-by: Alex Ernst <alex.m.ernst14@gmail.com>
@jtbandes
Copy link
Member

@alexern14 Thanks for getting this started and being patient! Now released in v2.1.0 🎉 https://mcap.dev/docs/typescript/classes/_mcap_core.McapWriter#InitializeForAppending

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtbandes jtbandes Awaiting requested review from jtbandes jtbandes is a code owner

@achim-k achim-k Awaiting requested review from achim-k achim-k is a code owner

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexern14 @jtbandes