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

[vpj] Improve error handling in VeniceVsonFileIterator and add tests #1406

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sushantmane
Copy link
Collaborator

@sushantmane sushantmane commented Dec 18, 2024

Fix NPE in VeniceVsonFileIterator when input file is not SequenceFile

  • Updated VeniceVsonFileIterator to fail loudly on file handle acquisition failures and
    ensure fileReader is properly initialized.
  • Improved error messages and logging to include detailed information and stack traces for
    easier debugging.
  • Added null check for VeniceVsonRecordReader in the constructor.
  • Added VeniceVsonFileIteratorTest to test initialization and handle exceptions.

How was this PR tested?

UT

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to explain your proposed changes and call out the behavior change.

@sushantmane sushantmane changed the title [vpj] Fix NPE in VeniceVsonFileIterator when input file is not SequenceFile [vpj] Improve error handling in VeniceVsonFileIterator and add tests Dec 18, 2024
…ition failures and

ensure fileReader is properly initialized.

- Improved error messages and logging to include detailed information and stack traces for
easier debugging.

- Added null check for VeniceVsonRecordReader in the constructor.

- Added VeniceVsonFileIteratorTest to test initialization and handle exceptions.
@sushantmane sushantmane force-pushed the fix-npe-in-VeniceVsonFileIterator branch from 903e1d6 to 6d0c4bc Compare December 19, 2024 00:10
String errorMessage =
String.format("Failed to open file: %s. Ensure that the file is a valid sequence file.", hdfsPath.getName());
LOGGER.error(errorMessage, e);
throw new VeniceException(errorMessage, e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the right thing to do

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me check how Avro input works. I'm very surprised this didn't fail for the avro inputs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does fail for avro input (non sequence file)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if this is the right thing to do

Why? If we can't create fileReader what's the point in continuing with the execution? We can add retries (for non-seq file exception) if we want to have additional layer of protection

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.

2 participants