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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

implement FiniteFirehoseFactory in InlineFirehose #8682

Merged
merged 2 commits into from
Oct 17, 2019
Merged

implement FiniteFirehoseFactory in InlineFirehose #8682

merged 2 commits into from
Oct 17, 2019

Conversation

aditya-r-m
Copy link
Contributor

Fixes #8673.

Description

Implementation of FiniteFirehoseFactory to support index_parallel tasks with inline data ingestion.
The implementation always returns 1 split.

This PR has:

  • been self-reviewed.
Key changed/added classes in this PR
  • InlineFirehoseFactory.java

@aditya-r-m
Copy link
Contributor Author

@ccaominh @jihoonson the approach mentioned on the ticket is implemented here.
I have tested it with basic inline ingestion tasks & will double check the behavior in a few more scenarios.

@jihoonson
Copy link
Contributor

@aditya-r-m thank you for your contribution! I have a couple of comments.

  • InlineFirehoseFactory should override boolean isSplittable() and return false because it's not splittable.
  • Would you please add some unit tests? Those test should verify InlineFirehoseFactory implements FiniteFirehoseFactory and isSplittable returns false. InlineFirehoseFactory is a good place for them.

@aditya-r-m
Copy link
Contributor Author

@jihoonson thank you for your guidance.
I will update the PR with the both of the points asap.

@aditya-r-m
Copy link
Contributor Author

@jihoonson implemented the suggested updates.

jihoonson
jihoonson approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@fjy fjy merged commit 75527f0 into apache:master Oct 17, 2019
@aditya-r-m aditya-r-m deleted the fix-inlineFireHoseParallelIndex branch October 17, 2019 07:22
jon-wei pushed a commit to jon-wei/druid that referenced this pull request Nov 27, 2019
@aditya-r-m @jon-wei
implement FiniteFirehoseFactory in InlineFirehose (apache#8682)
5adf4ea 
* implement FiniteFirehoseFactory in InlineFirehose

* override isSplittable in InlineFirehoseFactory & improve tests
@jon-wei jon-wei mentioned this pull request Nov 27, 2019
Merged
jon-wei added a commit that referenced this pull request Nov 27, 2019
@jon-wei
implement FiniteFirehoseFactory in InlineFirehose (#8682) (#8953)
ea60f9f 
* implement FiniteFirehoseFactory in InlineFirehose

* override isSplittable in InlineFirehoseFactory & improve tests
@jon-wei jon-wei mentioned this pull request Nov 29, 2019
Closed
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jihoonson jihoonson jihoonson approved these changes

Assignees
No one assigned
Labels
Bug Improvement
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

InlineFirehose does not work with index_parallel ingestion
4 participants
@aditya-r-m @jihoonson @fjy @jon-wei