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

Fix NPE when confirmationWindow is not initialized #82

Merged
merged 2 commits into from
Dec 26, 2020
Merged

Fix NPE when confirmationWindow is not initialized #82

merged 2 commits into from
Dec 26, 2020

Conversation

derrickoswald
Copy link
Contributor 

For cases where the throttle specifies a long time initially,
calls from Kafka to HttpSourceTask.commit() result in a NullPointerException
because the confirmationWindow initialization has not yet completed in HttpSourceTask.poll().
This adds a simple test and skips the update of offset if this is the case.

A short Debugging section was added to the README.md,
since this will be a common use-case for new deveopers to the project,
and some typos were corrected.

A few missing Maven <groupId> tags were added to avoid unresolved components.

Fix NPE when confirmationWindow is not initialized
6d66501 
    For cases where the throttle specifies a long time initially,
    calls from Kafka to HttpSourceTask.commit() result in a NullPointerException
    because the initialization has not yet completed in HttpSourceTask.poll().
    This adds a simple test and skips the update of offset if this is the case.
    A short Debugging section was added to the README.md,
    since this will be a common use-case for new deveopers to the project.
    A few missing Maven <groupId> tags were added to avoid unresolved components.
@castorm
Copy link
Owner

castorm commented Dec 26, 2020 •
edited
Loading

Hi @derrickoswald,

Thank you very much for your contributions. The debug section might definitely help some people.

Regarding the bugfix I'll leave some comments in the review, eager to hear your thoughts.

Best regards.

@castorm castorm added bug Something isn't working documentation Improvements or additions to documentation labels Dec 26, 2020
@castorm castorm added this to the v0.8.8 milestone Dec 26, 2020
Use fully constructed confirmationWindow instead of testing for null
92df251 
    On advice, removed the null test and replaced it with an initializer.
    Also removed unnecessary property declaration in pom.
@derrickoswald
Copy link
Contributor Author

I don't really have a test case, and I doubt I could integrate it in your mocking system.
In case it helps, I've attached the configuration.
Issue82.zip

@castorm castorm merged commit a8751e8 into castorm:master Dec 26, 2020
@castorm
Copy link
Owner

castorm commented Dec 26, 2020

Merged, test cased added, and about to release.

Thanks again @derrickoswald !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@castorm castorm castorm left review comments

Assignees
No one assigned
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Milestone
v0.8.8
Development

Successfully merging this pull request may close these issues.
2 participants
@derrickoswald @castorm