maytasm commented on a change in pull request #10467:
URL: https://github.com/apache/druid/pull/10467#discussion_r499808488
##########
File path:
core/src/main/java/org/apache/druid/data/input/impl/TimedShutoffInputSourceReader.java
##########
@@ -68,8 +68,10 @@ public TimedShutoffInputSourceReader(InputSourceReader
delegate, DateTime shutof
)
{
final Closer closer = Closer.create();
- closer.register(delegateIterator);
+ // We must register the shutdownNow method of the shutdown executor first.
This will cause the shutdown executor
+ // to be closed last and ensure other resources which depend on it can
close successfully.
closer.register(exec::shutdownNow);
Review comment:
1. Removed exec::shutdownNow from the closer in the
TimedShutoffInputSourceReader#decorateShutdownTimeout. Added a shutdown on the
exec instead to make sure that after the timeout period, the exec will
automatically be shutdown. This remove the dependency of shutting down the exec
from the CloseableIterator#close()
2. Made the recordSuppliers close function thread safe by adding synchronize
block. Left the Closers the same as it is also a documented feature/bug on
Guava's Closer it is not thread safe
(https://github.com/google/guava/issues/3900). We can keep this the same to
avoid confusion and possible update our Closer copy when the above issue is
done.
The CloseableIterator returned by
TimedShutoffInputSourceReader#decorateShutdownTimeout also protect against
concurrent close (scheduled exec vs normal closing) by using AtomicBoolean.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
