Move Streams.copy into elasticsearch-core and make a multi-release jar #29322
Conversation
This moves the method `Streams.copy(InputStream in, OutputStream out)` into the `elasticsearch-core` project (inside the `o.e.core.internal.io` package). It also makes this class into a multi-release class where the Java 9 equivalent uses `InputStream#transferTo`. This is a followup from elastic#29300 (comment)
dakrone
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
|
I want to make sure here I'm not missing something with the multi-release jar, is there testing for this other than CI JDK randomization? |
I don't believe we have any testing for this beyond the different JDK versions. |
| // we want to keep the JDKs in our IDEs set to JDK 8 until minimum JDK is bumped to 9 so we do not include this source set in our IDEs | ||
| if (!isEclipse && !isIdea) { | ||
| sourceSets { | ||
| java9 { |
There was a problem hiding this comment.
Technically this is java10 now, I think.
There was a problem hiding this comment.
Wouldn't that mean someone running with JDK 9 wouldn't use the 9-specific one? The code only uses Java 9 features so far so I figured that's where we should stick for now
There was a problem hiding this comment.
You can't build Elasticsearch with Java 9 as of last week I believe. That is all I'm getting at. I dunno if we want to bump all the java9 directories to java10 because of that. I think probably, but for now we can keep it.
There was a problem hiding this comment.
Even with java 10, we are still building this code with java 9 source level. It's really only about the standard features/libs that are allowed when compiling.
| out.flush(); | ||
| success = true; | ||
| return byteCount; | ||
| } finally { |
There was a problem hiding this comment.
I think that you can simplify all of this gross exception handling with:
try (in; out) {
final long byteCount = in.transferTo(out);
out.flush();
return byteCount;
}Note that this also uses a Java 9 language-level feature (a resource reference in the try-with-resources statement).
There was a problem hiding this comment.
See item 2 in JEP 213 for an explanation of this feature.
There was a problem hiding this comment.
It is also possible that
try (in;
out) {
}is clearer. I am not sure.
| * @return the number of bytes copied | ||
| * @throws IOException in case of I/O errors | ||
| */ | ||
| public static long copy(InputStream in, OutputStream out) throws IOException { |
There was a problem hiding this comment.
While JEP 213 does not require these to be final, it would be clearer if they were declared as such.
|
@jasontedor okay, I've updated this PR with your comments as well as the discussion we reached regarding not suppressing exceptions |
| * | ||
| * @see #close(Closeable...) | ||
| */ | ||
| public static void close(Exception ex, final Iterable<? extends Closeable> objects) throws IOException { |
There was a problem hiding this comment.
Would you make ex final please?
#29322) * Move Streams.copy into elasticsearch-core and make a multi-release jar This moves the method `Streams.copy(InputStream in, OutputStream out)` into the `elasticsearch-core` project (inside the `o.e.core.internal.io` package). It also makes this class into a multi-release class where the Java 9 equivalent uses `InputStream#transferTo`. This is a followup from #29300 (comment)
This moves the method
Streams.copy(InputStream in, OutputStream out)into theelasticsearch-coreproject (inside theo.e.core.internal.iopackage). Italso makes this class into a multi-release class where the Java 9 equivalent
uses
InputStream#transferTo.This is a followup from
#29300 (comment)