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

swift: flush writer buffer when closing a chunk #960

Merged
merged 2 commits into from
Aug 30, 2023
Merged

Conversation

jtbandes
Copy link
Member

Public-Facing Changes

Swift: data is now properly sent to the writer each time a chunk is closed.

Description

@@ -161,6 +161,9 @@ public final class MCAPWriter {
}
} else {
message.serialize(to: &buffer)
if buffer.count >= options.chunkSize {
Copy link
Member Author

Choose a reason for hiding this comment

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

Deceptively, this is actually for un-chunked writing – I thought I might as well use the chunkSize variable to decide when to flush, since it was there. Another option would be that we expose flush as a public method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have been resistant to exposing a public flush method in the go lib. I think we may in rust. I don't really understand the connection here though. If this feature is about unchunked writing why not just do buffer.count >= 0? caveat: I haven't properly

Copy link
Contributor

Choose a reason for hiding this comment

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

read the code... finished writing the sentence...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it would be flushing after every message - is that desirable? Maybe it's fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, the other line is necessary to ensure we flush when closing a chunk – this part is additionally adding a flush for non-chunked mode

@jtbandes jtbandes merged commit a68c769 into main Aug 30, 2023
24 checks passed
@jtbandes jtbandes deleted the jacob/swift-flush branch August 30, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants