You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by bdeggleston <gi...@git.apache.org> on 2016/02/29 19:58:22 UTC
[GitHub] incubator-tinkerpop pull request: 1185 - only load/instantiate the...
GitHub user bdeggleston opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/245
1185 - only load/instantiate the plugins listed in plugins.txt
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bdeggleston/incubator-tinkerpop 1185
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/245.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 #245
----
commit 698b0c0cce0211f14f66c1b7169e691507ca0ef9
Author: Blake Eggleston <bd...@gmail.com>
Date: 2016-02-29T18:57:26Z
1185 - only load/instantiate the plugins listed in plugins.txt
----
---
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: TINKERPOP-1185 - only load/insta...
Posted by bdeggleston <gi...@git.apache.org>.
Github user bdeggleston closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/245
---
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: 1185 - only load/instantiate the...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/245#issuecomment-190690137
Removing the `ServiceLoader` is a bigger change than it immediately appears. The `ServiceLoader` provides functionality that allows the console to show the user the list of plugins that are present but not "activated":
```text
gremlin> :plugin list
==>tinkerpop.server[active]
==>tinkerpop.gephi
==>tinkerpop.utilities[active]
==>tinkerpop.sugar
==>tinkerpop.credentials
==>tinkerpop.tinkergraph[active]
```
With this change in place you get:
```text
gremlin> :plugin list
==>tinkerpop.server[active]
==>tinkerpop.utilities[active]
==>tinkerpop.tinkergraph[active]
```
Of course, as we are learning, the notion of "active" is somewhat blurry because an "inactive" plugin may still load some aspect of a class during instantiation by `ServiceLoader`. An "inactive" plugin is really one that doesn't have its plugin methods called to perform imports, init the console environment, etc.
I guess the question is whether or not its valuable to know what plugins are available but not active. If it was determined that we could just drop that capability, there'd be some additional code/docs to amend to this PR. If we wanted to try to keep this functionality, i guess a custom `ServiceLoader` could work - it would just need to do the same thing as the current `ServiceLoader` but not throw an exception if a class could not be found - though that would have its own dangers in some sense.
@bdeggleston do you see any other ways to implement this without losing the functionality of listing available plugins as it works now?
---
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: 1185 - only load/instantiate the...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/245#issuecomment-190429620
Nice. Now I can actually simplify the docs pre-processor again.
VOTE: +1
---
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: 1185 - only load/instantiate the...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/245#issuecomment-191211675
The idea of "deferring" is interesting, but doesn't that just push the failure to when someone does an `:install` or `:plugin list`? won't the console come crashing down at that point? or is there something to be done to prevent that?
---
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: 1185 - only load/instantiate the...
Posted by bdeggleston <gi...@git.apache.org>.
Github user bdeggleston commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/245#issuecomment-190932147
I had a feeling this was too easy :)
So, as far as I can tell, there's no way to automagically detect `GremlinPlugin` implementations without loading and inspecting every class on the classpath. Internally, this is what `ServiceLoader` is doing, which causes static initialization of every class it loads.
What do you think about deferring the classpath scanning until `:plugin list` is run? I think the autodiscovery is useful, but it doesn't need to run on startup.
---
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: TINKERPOP-1185 - only load/insta...
Posted by bdeggleston <gi...@git.apache.org>.
Github user bdeggleston commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/245#issuecomment-191503050
It would also defer any failures for `:plugin list`, yes. `:plugin install` should be ok, assuming it only attempts to load the plugin class being installed.
To answer your question about preventing the failures during `:plugin list`: we should be able to catch exceptions, but we couldn't prevent any side effects (file io, etc) that occur during class initialization. That's another problem with deferring, `:plugin list` could end up having a lot of unintended side effects depending on what's on the classpath.
So then I guess it boils down to whether you think it's better to auto discover plugins, or better to skip loading unused classes.
---
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: TINKERPOP-1185 - only load/insta...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/245#issuecomment-207068865
I don't think there are any easy solutions here. I'll comment in more detail in JIRA but I'm not seeing how this gets done without considerable change to our approach to plugins. As long as we use `ServiceLoader` and require an instance of a plugin its going to load things that are inactive. I think we should probably just close this PR.
---
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.
---