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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle Arrow decoder errors gracefully #1247

Merged
merged 3 commits into from Dec 18, 2020
Merged

Handle Arrow decoder errors gracefully #1247

merged 3 commits into from Dec 18, 2020

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Dec 18, 2020 •
edited

馃摂 Description

This fixes a crash when encountering an issue with the on-disk format of serialized Arrow Record Batches or Arrow Schemas.

馃摑 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

馃幆 Review Instructions

Change random bytes from a segment and export contained table slices until you get the IOError from Arrow and notice that VAST doesn't crash anymore.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Dec 18, 2020
@dominiklohmann dominiklohmann requested a review from a team December 18, 2020 09:22
mavam
mavam requested changes Dec 18, 2020
View reviewed changes
libvast/src/arrow_table_slice.cpp Outdated Show resolved Hide resolved
libvast/src/arrow_table_slice.cpp Show resolved Hide resolved
libvast/src/arrow_table_slice.cpp Outdated Show resolved Hide resolved
@mavam
Copy link
Member

mavam commented Dec 18, 2020 •
edited

How can this be tested?

EDIT: maybe you can clarify review instrux?

@dominiklohmann
Copy link
Member Author

dominiklohmann commented Dec 18, 2020 •
edited

How can this be tested?

Change random bytes from a segment and export contained table slices until you get the IOError from Arrow and notice that VAST doesn't crash anymore.

EDIT: maybe you can clarify review instrux?

Edited in.

@dominiklohmann dominiklohmann force-pushed the topic/arrow-invalid-disk-state-crash branch from 1b2eb41 to 78a12f6 Compare December 18, 2020 13:27
@dominiklohmann
Copy link
Member Author

Rebased onto master.

@mavam mavam self-assigned this Dec 18, 2020
@mavam
Copy link
Member

mavam commented Dec 18, 2020

Shouldn't we do extra checking in L588 and L597?

mavam
mavam approved these changes Dec 18, 2020
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Turns out the answer is "no."

@mavam mavam merged commit 8028e20 into master Dec 18, 2020
@mavam mavam deleted the topic/arrow-invalid-disk-state-crash branch December 18, 2020 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavam mavam mavam approved these changes

Assignees

@mavam mavam

Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
2 participants
@dominiklohmann @mavam