ARROW-6867: [FlightRPC][Java] clean up default executor

[ghost]

We were feeding gRPC an executor, but neglecting to shut it down, which caused the JVM to hang at shutdown when running inside IntelliJ. The gRPC documentation requires that we shut down executors ourselves: https://grpc.github.io/grpc-java/javadoc/io/grpc/ServerBuilder.html#executor-java.util.concurrent.Executor-

Not too sure how to write a unit test for this - it clearly doesn't occur inside the Maven/JUnit unit test runner.

Travis: https://travis-ci.com/lihalite/arrow/builds/131648977

[github-actions]

https://issues.apache.org/jira/browse/ARROW-6867

[siddharthteotia]

If server.shutdown() fails, we will leak resources (fail to invoke AutoCloseables.close)?

[ghost]

We could, but server.shutdown doesn't throw any checked exceptions. I can move it into a finally block though.

[ghost]

Er, no, I can't, since I don't want to shut down the executor before the server itself shuts down.

[emkornfield]

why shutdownNow and not shutdown?

[ghost]

I was worried about it blocking but on reflection, if we make it to closing the executor, the gRPC server should be shut down and everything should be terminated. I've changed it and updated the comment to explain.

[emkornfield]

it seems like it would be clearer to remove the ternary operator combine the resources cleanup into one if/then block?

[ghost]

Yes, consolidated them.

[emkornfield]

Not too sure how to write a unit test for this - it clearly doesn't occur inside the Maven/JUnit unit test runner.

At the very least you should be able to test for the post condition that the executor passed in is terminated after a call to close.

[ghost]

Not too sure how to write a unit test for this - it clearly doesn't occur inside the Maven/JUnit unit test runner.

At the very least you should be able to test for the post condition that the executor passed in is terminated after a call to close.

Yes, I've added two tests to cover the two cases.

Apologies for the delay, I got tied up at work.

[emkornfield]

can't you try shutting down here and verify it does shutdown?

[ghost]

No, there's no accessible reference to the actual executor to confirm it afterwards.

[ghost]

I could plumb one through, but it'd be adding references specifically for testing.

(Sorry for the delay again, October was quite the busy month.)

[emkornfield]

What do you think about making resources less generic and take the reference to ExecutorService in the class instead of list of AutoCloseable? Then make awaitTermination rely on both the server and the executor (if its not null). That way you don't need anything specifically visible for testing.

[ghost]

That makes sense. I've updated the PR.

[emkornfield]

I think you could avoid exposing this if you added this to awaitTermination as well (but that is a little tricky to do correctly). I'm OK if you don't want to do it.

[ghost]

Hmm, how would that work in the test? We'd shut down the executor in awaitTermination and wait?

[emkornfield]

I would have thought we shutdown the executorservice in shutdown

[lidavidm]

Er, right. I can do that, but I still don't see how that'd let us avoid exposing it in test.

[emkornfield]

i think it tests the behavior implicitly vs explicitly as the way you have it now. Like I said I'm fine with either approach.

[emkornfield]

+1 merging.

[lidavidm]

Thank you! Apologies this dragged out so long.