Filesource compression support #5255
Conversation
Signed-off-by: Joël Marty <[email protected]>
Signed-off-by: Joël Marty <[email protected]>
Signed-off-by: Joël Marty <[email protected]>
Signed-off-by: Joël Marty <[email protected]>
Signed-off-by: Joël Marty <[email protected]>
joelmarty
requested review from
sb2k16,
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
Signed-off-by: Joël Marty <[email protected]>
joelmarty
force-pushed
the
filesource-compression-support
branch
from
c877f08 to
710dd13
Compare
Signed-off-by: Joël Marty <[email protected]>
joelmarty
force-pushed
the
filesource-compression-support
branch
from
74e0c6d to
4684124
Compare
| @@ -290,21 +321,18 @@ void setUp() { | |||
|
|
|||
| @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.
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.
| @@ -290,21 +321,18 @@ void setUp() { | |||
|
|
|||
| @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.
| @@ -10,3 +10,7 @@ Foundation (http://www.apache.org/). | |||
|
|
|||
| 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.
| @@ -10,3 +10,7 @@ Foundation (http://www.apache.org/). | |||
|
|
|||
| 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.
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.