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

AVRO-2944: Fix read pointer in java reader to avoid hang on partial read input stream #969

Merged
merged 2 commits into from
Nov 2, 2020

Conversation

MickJermsurawong
Copy link
Contributor

@MickJermsurawong MickJermsurawong commented Oct 24, 2020

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following one unit test to first illustrate failure

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@MickJermsurawong
Copy link
Contributor Author

The CI failure seems unrelated to the change in this PR.
For java 8:

Trevni Specification ............................... FAILURE [  3.264 s]

For java 11:

Updating dependencies
Your requirements could not be resolved to an installable set of packages.
  Problem 1
    - beberlei/composer-monorepo-plugin[v0.12, ..., v0.12.1] require composer-plugin-api ^1.0 -> found composer-plugin-api[2.0.0] but it does not match the constraint.
    - Root composer.json requires beberlei/composer-monorepo-plugin ^0.12 -> satisfiable by beberlei/composer-monorepo-plugin[v0.12, v0.12.1].

Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

LGTM! Good catch, and nice unit test, thanks for the contribution!

@mickjermsurawong-stripe

Hi @RyanSkraba just wondering if this patch could be back-ported to older versions too? We are using Iceberg that still uses Avro 1.9, and Hadoop with Avro 1.7.

@RyanSkraba
Copy link
Contributor

Hello! I got caught up taking a look at the build failure -- it's obviously nothing to do with your PR, but I would have liked to see it green.

There isn't an official 1.7.x release or even 1.9.3 planned as far as I know. We'd have to bring it up on the mailing list!

@Fokko
Copy link
Contributor

Fokko commented Nov 1, 2020

@MickJermsurawong can you rebase? The CI has been fixed on master

@github-actions github-actions bot added the Java Pull Requests for Java binding label Nov 2, 2020
@MickJermsurawong
Copy link
Contributor Author

Ah thank you Fokko for the note. Ryan, it's green now. Thank you for the review!
Will send out a note to dev list to see if folks have run into this problem.

@RyanSkraba RyanSkraba merged commit 942ec8e into apache:master Nov 2, 2020
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Nov 2, 2020
…ead input stream (apache#969)

* Add regression test to show failure before fixing

format test

* Fix: increment counter with number of bytes read
@RyanSkraba
Copy link
Contributor

Cherry-picked to branch-1.10! Thanks again for this fix!

henriquelsmti pushed a commit to henriquelsmti/avro that referenced this pull request Nov 3, 2020
…ead input stream (apache#969)

* Add regression test to show failure before fixing

format test

* Fix: increment counter with number of bytes read
@noslowerdna
Copy link
Contributor

@MickJermsurawong / @RyanSkraba I asked a question on the JIRA, curious if this fix could be improved to cover the read returning -1 unexpectedly as the issue description indicates it ought to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Pull Requests for Java binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants