Provide a way for ObjectReader to use a pre-defined executor

[kineshsatiya]

No description provided.

[jeremycnf]

Instead of the loop, you should probably just invoke executor.shutdownNow() if awaitTermination doesn't return or is itself interrupted, e.g. someone called shutdownNow() on ObjectReader's thread. Something like this:

                finally {
                    executor.shutdown();
                    try {
                        if (!executor.awaitTermination(maxWaitTimeInSeconds, TimeUnit.SECONDS)) {
                            throw new Exception("Took too long to fetch object: " + objectName);
                        }
                    } finally{
                        if(!executor.isTerminated()) {
                            executor.shutdownNow();
                        }
                    }
                }

[mfiguiere]

I agree, swallowing the InterruptedException here seems inappropriate. But I'm also wondering if it's appropriate for the ObjectReader to shutdown itself the executor when it has been provided through withExecutorService(). Shuting it down was making sense when the ObjectReader was creating it itself. But it feels to me that some developer would expect this method to be a way to provide a long running, shared and reused ExecutorService and therefore expect the ObjectReader class to not shut it down.

[jeremycnf]

Agree with @mfiguiere that generally any logic which creates an executor should be responsible for its life-cycle.

@kineshsatiya, is the new feature to set an external executor absolutely required for the shutdown bug fix?

We need to restrict master to only critical bug fixes. The ability to pass an external executor seems more like a (likely) small performance optimization given that ObjectReader's use case is transferring large objects.

[kineshsatiya]

thank you @jeremycnf and @mfiguiere for quick reviews.

The original intent for users to be able to pass an external executor to ObjectReader is performance related ( to avoid having zombie threads). Calling shutdown with external executor service would have defeated that purpose. I've made the changes that does not shutdown the external service.

[meleshuk]

I think this call merits some diagnostics - logging what it returns and whether or not the tasks have actually terminated. Here is what shutdownNow() documentation says:

"This method does not wait for actively executing tasks to terminate. Use awaitTermination to do that.

There are no guarantees beyond best-effort attempts to stop processing actively executing tasks. For example, typical implementations will cancel via Thread.interrupt(), so any task that fails to respond to interrupts may never terminate."

I know I was the one who suggested it, but it seems like that even with this change (which I believe should help a lot), there is still potential for getting into thread count explosion. I think it would be good to at least wait for a few seconds here and log whether or not threads have terminated.

(Completely agree with others on inappropriateness of shutting down a shared executor - it could be used in parallel by multiple ObjectReaders with clearly unexpected results.)

[jeremycnf]

+1 Logging is a good idea.

The only reliable way to kill executor threads still not terminated after shutdownNow() is to kill the JVM itself; in such case we'd need to further investigate whether this recipe is the root cause. If the block is due to an optionally-specified callback, the caller would need to investigate their callback logic.