-
Notifications
You must be signed in to change notification settings - Fork 188
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
Fixes a bug in the S3 sink where events without handles throw NPE #2814
Fixes a bug in the S3 sink where events without handles throw NPE #2814
Conversation
…NPEs by skipping any such handles. Signed-off-by: David Venable <dlv@amazon.com>
@@ -106,7 +106,9 @@ void output(Collection<Record<Event>> records) { | |||
final byte[] encodedBytes = encodedEvent.getBytes(); | |||
|
|||
currentBuffer.writeEvent(encodedBytes); | |||
bufferedEventHandles.add(event.getEventHandle()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a unit test to confirm this is fixed? And help prevent future NPEs around this leak into this plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually tried this, but I was not able to get it to crash such that the test would actually show any verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null event handle would be the default case when not using end-to-end acknowledgements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmanning09 , I found that there was an unnecessary try-catch on NullPointerException
. I removed that and was able to add a valid test.
…rupt the thread on exceptions. Signed-off-by: David Venable <dlv@amazon.com>
…ensearch-project#2814) Fixes a bug in the S3 sink where events without handles are throwing NPEs by skipping any such handles. Signed-off-by: David Venable <dlv@amazon.com> Signed-off-by: Marcos_Gonzalez_Mayedo <alemayed@amazon.com>
Description
Fixes a bug in the
s3
sink for null Event handles.Issues Resolved
N/A
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.