You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by pingtimeout <gi...@git.apache.org> on 2015/11/18 15:22:49 UTC
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
GitHub user pingtimeout opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/146
Close Cluster executor once the cluster is closed (TINKERPOP3-972)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pingtimeout/incubator-tinkerpop TINKERPOP3-972
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/146.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #146
----
commit 774282ca0e651f1123abe25be4ef9ee421c2eead
Author: Pierre Laporte <pi...@pingtimeout.fr>
Date: 2015-11-18T14:20:24Z
Close Cluster executor once the cluster is closed (TINKERPOP3-972)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by pingtimeout <gi...@git.apache.org>.
Github user pingtimeout commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157746085
I think this case is exactly the reason why methods like `shutdown()` exist independently from `shutdownNow()`. As you mentionned, adding `.awaitTermination(...)` would make the async call synchronous.
I can only see 3 use cases here, and all of them are covered:
1. I want a synchronous shutdown, I should use `cluster.close()`
2. I want a asynchronous shutdown, I should use `cluster.closeAsync()`
3. I want a timely asynchronous shutdown, I should use `cluster.closeAsync().get(5, TimeUnit.SECONDS)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by pingtimeout <gi...@git.apache.org>.
Github user pingtimeout commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157756815
I am not an expert in the Tinkerpop codebase but my understanding of the code is the following:
Yes, the `CompletableFuture` can be executed in parallel with other tasks by the executor, since [it may be](https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java#L580-580) multithreaded. However, I don't think it can complete if tasks are still being executed. It calls `factory.shutdown()` which [delegates the action](https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java#L552-552) to Netty.
That `factory.shutdown()` closes Netty's event loop after a grace delay of roughly 17 seconds (that's the "sensible values" described by `EventLoopGroup::shutdownGracefully()`. A long running task that uses the transport layer may therefore encounter an IOException, but I don't think that is a really bad thing, considering it is the user who explicitly triggered the `cluster.close()` without waiting for all of the pending requests to complete.
I mean, as a developer, if my code is reading a file and I call `inputStream.close()` in another thread, I don't expect the latter call to block indefinitely until the whole file has been read.
In the alternative approach you describe, how long would you wait for the termination to happen?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by pingtimeout <gi...@git.apache.org>.
Github user pingtimeout commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157740699
Thanks for the review. Indeed I forgot the async version of `close()`.
We could use `CompletableFuture::thenRun(Runnable, Executor)` with the common Fork/Join pool to shut down the executor but it may take a while if that pool is overwhelmed...
I am trying another implementation where `executor().shutdown()` is called [immediately after](https://github.com/apache/incubator-tinkerpop/blob/0feaa8b47de2797e177310cd1ea3bf38d7230420/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java#L619) the call to `.submit()` returns. According to the Javadoc of `ExecutorService`, it should work:
Initiates an orderly shutdown in which previously submitted tasks are executed, but no new tasks will be accepted.
If the build succeeds, I will replace my previous commit with that new change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157750708
Ok - so just to be clear, for your cases 2 and 3, the executor that you called `shutdown()` on might still be active when the `CompletableFuture` finishes to say the "cluster is closed"? In other words, all resources haven't been really shutdown at the point the future returns because the executor might still be working on that since you aren't blocking. Is that a problem or am I not thinking of that right?
actually, I'm trying to think of what weird things might happen if there is something long running in the executor and then we call closeAsync()? If there was such a situation, that will schedule a job to close the transport which might somehow be in use by that long run job. It seems like it is wrong to try to use the executor in shutdown. Maybe it would be better if it was just a standalone "shutdown" thread? in that way it could call `executor.shutdown()` and `awaitTermination()` and then call `factory.shutdown()` and then complete the future. wdyt? am i thinking too much?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157733926
Thanks for noticing this and offering a fix. Somehow I missed closing that executor.
Looking at your PR, you do a `executor().shutdownNow()` in `Cluster.close()`. That still leaves a problem for those doing `Cluster.closeAsync()` as that executor will continue to hang around. Maybe your change should somehow be down here:
https://github.com/apache/incubator-tinkerpop/blob/0feaa8b47de2797e177310cd1ea3bf38d7230420/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java#L607
maybe you tack something on to the `closeIt` future to shutdown the executor after if finishes all its jobs? or maybe we don't use the executor for shutdown of the channel so that we separate that task from any other running activities? do you have any thoughts on how best to do this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157743674
yeah - that would be another way. would you also block (i.e. `awaitTermination()`) in that method when you call `shutdown()`? That's not so good for `closeAsync()` if we block i guess....
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157757921
> I don't expect the latter call to block indefinitely until the whole file has been read.
What you wrote above makes sense to me - I just wanted to work through all the ramifications of `close()` to make sure we were doing what devs would expect...Thanks for talking that through with me. I'll merge shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/146#issuecomment-157901889
This was a minor change - one line of code - merged by CTR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: Close Cluster executor once the ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/146
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---