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

Fixed a bug in HttpPostEmitter leading to ClassCastException #8205

Merged
merged 4 commits into from
Aug 1, 2019

Conversation

ArtyomyuS
Copy link
Contributor

@ArtyomyuS ArtyomyuS commented Jul 31, 2019

Fixes #8204

Description

Fix HttpPostEmitter class cast exception while trying to recover a batch. The fix is straightforward just changed Integer check for a batch number to recover into Long.


This PR has:

  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.

For reviewers: the key changed/added classes in this PR are HttpPostEmitter and HttpPostEmitterTest.

(Add this section in big PRs to ease navigation in them for reviewers.)

Copy link
Member

@leventov leventov left a comment

Choose a reason for hiding this comment

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

@ArtyomyuS could you please apply the same changes in comments as I did in https://github.com/apache/incubator-druid/pull/8207/files?

@leventov leventov changed the title Issue 8206: Fixed class cast exception in case of batch recovery Fixed class cast exception in HttpPostEmitter Jul 31, 2019
@leventov leventov changed the title Fixed class cast exception in HttpPostEmitter Fixed a bug in HttpPostEmitter leading to ClassCastException Jul 31, 2019
@ArtyomyuS
Copy link
Contributor Author

@leventov I updated. Thanks.

@clintropolis clintropolis added this to the 0.15.1 milestone Aug 1, 2019
@Test
@SuppressWarnings("unchecked")
public void testRecoveryEmitAndReturnBatch()
throws InterruptedException, IOException, NoSuchFieldException, IllegalAccessException
Copy link
Member

@leventov leventov Aug 1, 2019

Choose a reason for hiding this comment

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

Side note: in Druid, we usually just add throws Exception if there is more than one. I don't think there may be any harm in doing that.

Copy link
Member

@leventov leventov 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 patch with a test!

@leventov leventov merged commit bcaffe2 into apache:master Aug 1, 2019
leventov pushed a commit to metamx/druid that referenced this pull request Aug 1, 2019
…8205)

* Issue 8206: Fixed class cast exception in case of batch recovery

* Issue 8206: Added HttpPostEmitterTest license header

* Issue 8206: Updated comments accordingly to code review.

* Issue 8206: Updated HttpPostEmitterTest accordingly to new modifications.
clintropolis pushed a commit that referenced this pull request Aug 1, 2019
…8220)

* Issue 8205: Fixed class cast exception in case of batch recovery

* Issue 8205: Added HttpPostEmitterTest license header

* Issue 8205: Updated comments accordingly to code review.

* Issue 8205: Updated HttpPostEmitterTest accordingly to new modifications.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpPostEmitter throw Class cast exception when using emitAndReturnBatch
3 participants