Filesource compression support#5255
Conversation
Signed-off-by: Joël Marty <jmarty@twilio.com>
Signed-off-by: Joël Marty <jmarty@twilio.com>
Signed-off-by: Joël Marty <jmarty@twilio.com>
Signed-off-by: Joël Marty <jmarty@twilio.com>
Signed-off-by: Joël Marty <jmarty@twilio.com>
c877f08 to
710dd13
Compare
Signed-off-by: Joël Marty <jmarty@twilio.com>
74e0c6d to
4684124
Compare
| @Test | ||
| void start_will_parse_codec_with_correct_inputStream() throws IOException { | ||
| createObjectUnderTest().start(buffer); | ||
| final FileInputStream decompressedStream = new FileInputStream(TEST_FILE_PATH_PLAIN); |
There was a problem hiding this comment.
Rewrote the test: previous implementation worked by testing equality between the bytes produced by the source and the bytes produced by reading the file directly.
The test now checks that the codec is called with the inputstream returned by the decompression engine.
There was a problem hiding this comment.
Rather than changing the whole test, could you just mock the DecompressionEngine to return exactly the bytes given?
Perhaps an alternative is to keep both tests.
There was a problem hiding this comment.
I cannot keep the test as it is: the test relies on the previous implementation's behavior: it tries to read from the input stream created in FileSource.CodecFileStrategy#start but because this stream is created in a try-with-resource (see FileSource.java L181), it is closed when the test tries to read it again on FileSourceTest.java L303.
In other words, the test used to work because the input stream was never properly closed in the previous implementation of the WithCodec strategy. I could reintroduce what I believe is a bug to keep the test as-is, but IMHO it would make more sense to validate the test is equivalent to the previous implementation.
dlvenable
left a comment
There was a problem hiding this comment.
Thank you @joelmarty for this contribution! I have one comment on the tests, but the rest looks good.
| @Test | ||
| void start_will_parse_codec_with_correct_inputStream() throws IOException { | ||
| createObjectUnderTest().start(buffer); | ||
| final FileInputStream decompressedStream = new FileInputStream(TEST_FILE_PATH_PLAIN); |
There was a problem hiding this comment.
Rather than changing the whole test, could you just mock the DecompressionEngine to return exactly the bytes given?
Perhaps an alternative is to keep both tests.
NOTICE
Outdated
| This product includes software developed by | ||
| Joda.org (http://www.joda.org/). | ||
|
|
||
| This product includes software developed by |
There was a problem hiding this comment.
Is this a requirement from your company? We don't require contributors to add to this.
There was a problem hiding this comment.
Please see if your company requires this. If not, it would be best to remove this change.
In the meantime, I'm also checking with the OpenSearch project to determine if this is an acceptable change or not.
There was a problem hiding this comment.
Yes, they asked me to make this change. According to our OSS team, it's how the company can retain the copyright for my contributions as an employee.
I am not sure whether the notice file is the appropriate place to capture this, the information I found after some search tends to say it is not.
I think it is better to get some guidance.
There was a problem hiding this comment.
Hi @dlvenable , any news on this ? The PR has been marked as accepted by @JonahCalvo but there are failing build checks and I cannot merge it myself.
NOTICE
Outdated
| This product includes software developed by | ||
| Joda.org (http://www.joda.org/). | ||
|
|
||
| This product includes software developed by |
There was a problem hiding this comment.
Please see if your company requires this. If not, it would be best to remove this change.
In the meantime, I'm also checking with the OpenSearch project to determine if this is an acceptable change or not.
Signed-off-by: Joël Marty <134835+joelmarty@users.noreply.github.com>
NOTICE changes are intended to preserve copyrights in place at the time of application of apache license, it is not intended to capture "copyrights" for new contributors.
|
@JonahCalvo @dlvenable I resolved the merge conflict and reverted the changes in the NOTICE file. Could you please review again and if approved, merge the PR please ? (I cannot do that myself) |
|
@joelmarty , Sorry for the delay on this. I think I missed your most recent changes. We have merged this since you removed the NOTICE updates. Thank you. |
Add support for compressed files in FileSource Signed-off-by: Joël Marty <jmarty@twilio.com> Signed-off-by: Joël Marty <134835+joelmarty@users.noreply.github.com>
Add support for compressed files in FileSource Signed-off-by: Joël Marty <jmarty@twilio.com> Signed-off-by: Joël Marty <134835+joelmarty@users.noreply.github.com> Signed-off-by: Nathan Wand <wandna@amazon.com>
Description
Add support for compressed files in file source.
Issues Resolved
Fixes #5245
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.