Discard Stream cache and expose StreamSupplier #342
Conversation
| @@ -25,7 +25,10 @@ | |||
| import java.util.stream.Stream; | |||
|
|
|||
| /** | |||
| * A JPAStreamer is responsible for creating Streams from data sources | |||
| * A JPAStreamer is responsible for creating Streams from data sources, | |||
| * alternatively for creating Streamers that can be reused to create Streams of the same Entity source, | |||
There was a problem hiding this comment.
Thanks, a relic of my first attempt to introduce Streamers.
| * If you are using the same Stream source frequently e.g. Film.class, | ||
| * consider configuring a {@link StreamSupplier} that can supply | ||
| * {@link Stream}s from the same source over and over again. | ||
| * This save resources and avoids instantiating a new {@link EntityManager} for each new {@link Stream}s. |
There was a problem hiding this comment.
for each new Streams (singularis)
| * <p> | ||
| * Here is an example of using a {@link StreamSupplier}: | ||
| * <pre>{@code | ||
| * final StreamSupplier<Film> streamSupplier = jpaStreamer.createStreamSupplier(Film.class); |
There was a problem hiding this comment.
I think using Try-With-Resources would be better here.
There was a problem hiding this comment.
Could I provide both examples? I just think this way is clearer, but I totally get that TWR is a better practice.
| * @author Per Minborg, Julia Gustafsson | ||
| * @since 3.0.1 | ||
| */ | ||
| public interface StreamSupplier<T> { |
There was a problem hiding this comment.
add implements AutoCloseable allowing TWR use.
There was a problem hiding this comment.
Do you mean extends AutoCloseable?
| @@ -33,7 +34,7 @@ final class StandardJPAStreamer implements JPAStreamer { | |||
|
|
|||
| private final Supplier<EntityManager> entityManagerSupplier; | |||
| private final Runnable closeHandler; | |||
| private final Map<StreamConfiguration<?>, Streamer<?>> streamerCache; | |||
| private final Map<StreamConfiguration<?>, StreamSupplier<?>> streamerCache; | |||
There was a problem hiding this comment.
I think now we can delete this field.
| } | ||
|
|
||
| @Override | ||
| public void resetStreamer(Class<?>... entityClasses) throws UnsupportedOperationException { |
There was a problem hiding this comment.
We should not declare throws UOE
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| streamerCache.values().forEach(Streamer::close); | ||
| streamerCache.values().forEach(StreamSupplier::close); |
| public void close() { | ||
| //System.out.println("Closing Streamer<" + entityClass.getSimpleName() + ">"); | ||
| renderer.close(); | ||
| public void close() throws UnsupportedOperationException { |
| if (this.closeEntityManager) { | ||
| renderer.close(); | ||
| } else { | ||
| System.out.println("JPAStreamer does not close Entity Managers obtained by a given Supplier<EntityManager>."); |
There was a problem hiding this comment.
Maybe we should only print this message once. So we could have a static field of type AtomicBoolean that controls this.
There was a problem hiding this comment.
I think we need to discuss this briefly. This slight complexity arose due to compatibility with Quarkus.
The current way we cache Streams can cause memory leaks, see #147. Therefore we suggest removing the
StreamSuppliercache and instead allowing users to cacheStreamSupplierif frequently reusing the same Stream source.