[GitHub] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/569 Nice to see that all the tests are passing. Thanks for doing that. I've given this a quick review and have a few top-level comments: * Note that our code style is to explicitly use `final` for all variable declarations that require it - i think we make exceptions on exceptions in catch blocks and the var declaration of a `for()` loop, but that's about it. I think you've missed a few of those. * Could you please rename `BasicGraphManager` to `DefaultGraphManager`? We only use the term "Basic" in one other class naming, so it's a bit of an anomaly compared to "Default". * Does this change introduce any compile-time breaking changes for anyone who has depended on the `GraphManager` class (or other things you've modified)? I'm wondering if this is a change better suited for the master branch and 3.3.0. If it stays on `tp32` I may have to ask you to provide a second pull request to master that can also be evaluated as part of a second review/vote. * The addition of `addGraph()` is nice. * Can you elaborate on the usage of `openGraph()` - I think I get it, but it's not clear to me based on the javadoc/default implementation * Please add a entry (or more) to CHANGELOG for these updates. * Please update the reference documentation to include your new Gremlin Server configuration option here: http://tinkerpop.apache.org/docs/current/reference/#_configuring_2 - not sure how much you need to write here. Users of `GraphManager` have likely dug pretty deep into the code and will see how it works. Perhaps include a link to the javadoc for `GraphManager` and beef up the class javadoc there so that implementers know what to do. Just a bit of warning - from my perspective, this is a fairly involved pull request you've started (and thanks!) so it will likely take a some time and iterations to get through before we can get it merged. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1438) Consider GraphManager as an interface
[
https://issues.apache.org/jira/browse/TINKERPOP-1438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15904971#comment-15904971
]
ASF GitHub Bot commented on TINKERPOP-1438:
---
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
Nice to see that all the tests are passing. Thanks for doing that.
I've given this a quick review and have a few top-level comments:
* Note that our code style is to explicitly use `final` for all variable
declarations that require it - i think we make exceptions on exceptions in
catch blocks and the var declaration of a `for()` loop, but that's about it. I
think you've missed a few of those.
* Could you please rename `BasicGraphManager` to `DefaultGraphManager`? We
only use the term "Basic" in one other class naming, so it's a bit of an
anomaly compared to "Default".
* Does this change introduce any compile-time breaking changes for anyone
who has depended on the `GraphManager` class (or other things you've
modified)? I'm wondering if this is a change better suited for the master
branch and 3.3.0. If it stays on `tp32` I may have to ask you to provide a
second pull request to master that can also be evaluated as part of a second
review/vote.
* The addition of `addGraph()` is nice.
* Can you elaborate on the usage of `openGraph()` - I think I get it, but
it's not clear to me based on the javadoc/default implementation
* Please add a entry (or more) to CHANGELOG for these updates.
* Please update the reference documentation to include your new Gremlin
Server configuration option here:
http://tinkerpop.apache.org/docs/current/reference/#_configuring_2 - not sure
how much you need to write here. Users of `GraphManager` have likely dug pretty
deep into the code and will see how it works. Perhaps include a link to the
javadoc for `GraphManager` and beef up the class javadoc there so that
implementers know what to do.
Just a bit of warning - from my perspective, this is a fairly involved pull
request you've started (and thanks!) so it will likely take a some time and
iterations to get through before we can get it merged.
> Consider GraphManager as an interface
> -
>
> Key: TINKERPOP-1438
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1438
> Project: TinkerPop
> Issue Type: Improvement
> Components: server
>Affects Versions: 3.2.2
>Reporter: stephen mallette
>Priority: Minor
> Labels: breaking
>
> If {{GraphManager}} were an interface it would make embedding Gremlin Server
> easier as {{Graph}} instances could be more easily supplied by the host
> application. In doing this, It also might be good to force a
> {{TraversalSource}} to be referred to by both the {{Graph}} name and
> {{TraversalSource}} name.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Github user dpitera commented on the issue: https://github.com/apache/tinkerpop/pull/569 > Does this change introduce any compile-time breaking changes for anyone who has depended on the GraphManager class (or other things you've modified)? I believe the answer is no (in the non-mutated use of pure tinkerpop) because: 1. The previous implementation's graphManager instantiation could only be retrieved by two places: 1. The [ServerGremlinExecutor](https://github.com/apache/tinkerpop/blob/ad51fb24eb4d621763f6b3c02029daa54e7dc20d/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java#L202) which is only accessible through the [GremlinServer](https://github.com/apache/tinkerpop/blob/c5b9680e70d696d000b945abbd72a3b8a3f97256/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java#L328) which users do not have access to the instantiation unless they have modified their implementor's logic to define its own "GremlinServer" and not use Tinkerpop's 2. The [Context](https://github.com/apache/tinkerpop/blob/4293eb333dfbf3aea19cd326f9f3d13619ac0b54/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java#L80) which also is instantiated by the same graphManager pulled from the servergremlinexecutor, and the Context class is final (and thus cannot be extended) and there is no way to retrieve that `Context` in the code. Therefore, i am not sure who would have been able to make use of the graphManager themselves without using a modified version of tinkerpop. > Can you elaborate on the usage of openGraph() The `addGraph()` and `getGraph()` are to act as modifiers to the object holding the graph references, while `opengraph()` allows for a mechanism by which to instantiate a graph dynamically through the graphManager (and thus allow for reference tracking of it) that is not currently in said graphManager's reference tracking object. The use of a `Supplier` here allows for the implementor to define his/her own custom function by which to instantiate the graph > If it stays on tp32 I may have to ask you to provide a second pull request to master that can also be evaluated as part of a second review/vote. I would say once we get this one merged, then we can open the PR on master, but I would like to get it into the next release as well > it will likely take a some time and iterations to get through before we can get it merged Sounds good. I usually prefer quality to expediency. When I get around to fixing up some of your requests, I will leave a comment here saying I have pushed new code. Thanks for your review. Let me know if you have more questions! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1438) Consider GraphManager as an interface
[
https://issues.apache.org/jira/browse/TINKERPOP-1438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905268#comment-15905268
]
ASF GitHub Bot commented on TINKERPOP-1438:
---
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> Does this change introduce any compile-time breaking changes for anyone
who has depended on the GraphManager class (or other things you've modified)?
I believe the answer is no (in the non-mutated use of pure tinkerpop)
because:
1. The previous implementation's graphManager instantiation could only be
retrieved by two places:
1. The
[ServerGremlinExecutor](https://github.com/apache/tinkerpop/blob/ad51fb24eb4d621763f6b3c02029daa54e7dc20d/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ServerGremlinExecutor.java#L202)
which is only accessible through the
[GremlinServer](https://github.com/apache/tinkerpop/blob/c5b9680e70d696d000b945abbd72a3b8a3f97256/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java#L328)
which users do not have access to the instantiation unless they have modified
their implementor's logic to define its own "GremlinServer" and not use
Tinkerpop's
2. The
[Context](https://github.com/apache/tinkerpop/blob/4293eb333dfbf3aea19cd326f9f3d13619ac0b54/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Context.java#L80)
which also is instantiated by the same graphManager pulled from the
servergremlinexecutor, and the Context class is final (and thus cannot be
extended) and there is no way to retrieve that `Context` in the code.
Therefore, i am not sure who would have been able to make use of the
graphManager themselves without using a modified version of tinkerpop.
> Can you elaborate on the usage of openGraph()
The `addGraph()` and `getGraph()` are to act as modifiers to the object
holding the graph references, while `opengraph()` allows for a mechanism by
which to instantiate a graph dynamically through the graphManager (and thus
allow for reference tracking of it) that is not currently in said
graphManager's reference tracking object. The use of a `Supplier` here allows
for the implementor to define his/her own custom function by which to
instantiate the graph
> If it stays on tp32 I may have to ask you to provide a second pull
request to master that can also be evaluated as part of a second review/vote.
I would say once we get this one merged, then we can open the PR on master,
but I would like to get it into the next release as well
> it will likely take a some time and iterations to get through before we
can get it merged
Sounds good. I usually prefer quality to expediency. When I get around to
fixing up some of your requests, I will leave a comment here saying I have
pushed new code.
Thanks for your review. Let me know if you have more questions!
> Consider GraphManager as an interface
> -
>
> Key: TINKERPOP-1438
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1438
> Project: TinkerPop
> Issue Type: Improvement
> Components: server
>Affects Versions: 3.2.2
>Reporter: stephen mallette
>Priority: Minor
> Labels: breaking
>
> If {{GraphManager}} were an interface it would make embedding Gremlin Server
> easier as {{Graph}} instances could be more easily supplied by the host
> application. In doing this, It also might be good to force a
> {{TraversalSource}} to be referred to by both the {{Graph}} name and
> {{TraversalSource}} name.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/569 > Therefore, i am not sure who would have been able to make use of the graphManager themselves without using a modified version of tinkerpop. You would be surprised by what folks manage to make use of in their code. If it's public, people will use it. :smile: anyway, i mostly wanted to be sure we don't introduce any compile time breaks when folks go to use 3.2.5. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1438) Consider GraphManager as an interface
[
https://issues.apache.org/jira/browse/TINKERPOP-1438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905295#comment-15905295
]
ASF GitHub Bot commented on TINKERPOP-1438:
---
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> Therefore, i am not sure who would have been able to make use of the
graphManager themselves without using a modified version of tinkerpop.
You would be surprised by what folks manage to make use of in their code.
If it's public, people will use it. :smile: anyway, i mostly wanted to be sure
we don't introduce any compile time breaks when folks go to use 3.2.5.
> Consider GraphManager as an interface
> -
>
> Key: TINKERPOP-1438
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1438
> Project: TinkerPop
> Issue Type: Improvement
> Components: server
>Affects Versions: 3.2.2
>Reporter: stephen mallette
>Priority: Minor
> Labels: breaking
>
> If {{GraphManager}} were an interface it would make embedding Gremlin Server
> easier as {{Graph}} instances could be more easily supplied by the host
> application. In doing this, It also might be good to force a
> {{TraversalSource}} to be referred to by both the {{Graph}} name and
> {{TraversalSource}} name.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (TINKERPOP-1438) Consider GraphManager as an interface
[
https://issues.apache.org/jira/browse/TINKERPOP-1438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905302#comment-15905302
]
ASF GitHub Bot commented on TINKERPOP-1438:
---
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/569
It would break if somebody was calling on `new GraphManager(Settings)`. You
could change the interface to `IGraphManager` and have `GraphManager` implement
it.
> Consider GraphManager as an interface
> -
>
> Key: TINKERPOP-1438
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1438
> Project: TinkerPop
> Issue Type: Improvement
> Components: server
>Affects Versions: 3.2.2
>Reporter: stephen mallette
>Priority: Minor
> Labels: breaking
>
> If {{GraphManager}} were an interface it would make embedding Gremlin Server
> easier as {{Graph}} instances could be more easily supplied by the host
> application. In doing this, It also might be good to force a
> {{TraversalSource}} to be referred to by both the {{Graph}} name and
> {{TraversalSource}} name.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Github user pluradj commented on the issue: https://github.com/apache/tinkerpop/pull/569 It would break if somebody was calling on `new GraphManager(Settings)`. You could change the interface to `IGraphManager` and have `GraphManager` implement it. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop pull request #567: TINKERPOP-1644 Improve script compilation syncr...
Github user BrynCooke closed the pull request at: https://github.com/apache/tinkerpop/pull/567 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1644) Improve script compilation syncronisation
[ https://issues.apache.org/jira/browse/TINKERPOP-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905314#comment-15905314 ] ASF GitHub Bot commented on TINKERPOP-1644: --- Github user BrynCooke closed the pull request at: https://github.com/apache/tinkerpop/pull/567 > Improve script compilation syncronisation > - > > Key: TINKERPOP-1644 > URL: https://issues.apache.org/jira/browse/TINKERPOP-1644 > Project: TinkerPop > Issue Type: Improvement > Components: groovy >Affects Versions: 3.2.4 >Reporter: Bryn Cooke >Assignee: stephen mallette > > Currently there is no synchronisation around script compilation. This means > that if a particularly heavy script is in use, many threads may end up > compiling the same script. > It would seem like a good idea to have some some sort of synchronisation to > prevent ever getting to this stage. > In addition, there will be cases where users will repeatedly submit broken > scripts to the server. In this case it is useful to log the error the first > time the script compilation is attempted and then cache the error for > subsequent runs. > Finally I have found some scripts take in excess of 30 seconds to compile. To > aid performance debugging the script compilation times should be included in > the logs. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[GitHub] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Github user dpitera commented on the issue: https://github.com/apache/tinkerpop/pull/569 > It would break if somebody was calling on new GraphManager(Settings) If someone were doing this, it would mean they have had to reconstruct a `Settings` object, which would be different than that instantiated inside the `GremlinServer` and I would argue this is _outside the scope_ of something we need to be compatible with, i.e. using a modified version of Tinkerpop, or using it in an unexpected way. What are your thoughts here? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1438) Consider GraphManager as an interface
[
https://issues.apache.org/jira/browse/TINKERPOP-1438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905324#comment-15905324
]
ASF GitHub Bot commented on TINKERPOP-1438:
---
Github user dpitera commented on the issue:
https://github.com/apache/tinkerpop/pull/569
> It would break if somebody was calling on new GraphManager(Settings)
If someone were doing this, it would mean they have had to reconstruct a
`Settings` object, which would be different than that instantiated inside the
`GremlinServer` and I would argue this is _outside the scope_ of something we
need to be compatible with, i.e. using a modified version of Tinkerpop, or
using it in an unexpected way. What are your thoughts here?
> Consider GraphManager as an interface
> -
>
> Key: TINKERPOP-1438
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1438
> Project: TinkerPop
> Issue Type: Improvement
> Components: server
>Affects Versions: 3.2.2
>Reporter: stephen mallette
>Priority: Minor
> Labels: breaking
>
> If {{GraphManager}} were an interface it would make embedding Gremlin Server
> easier as {{Graph}} instances could be more easily supplied by the host
> application. In doing this, It also might be good to force a
> {{TraversalSource}} to be referred to by both the {{Graph}} name and
> {{TraversalSource}} name.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] tinkerpop issue #569: TINKERPOP-1438: GraphManager becomes a customizable in...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/569 yeah - that's an obvious truth @pluradj . Normally we wouldn't allow such a change on `tp32` because it does immediately make people's code not compile if they used that class in that way but this class is fairly internal. If someone is using it they likely know what they are doing. @dpitera I tend to agree that this is outside the scope of what would need to remain compatible. That said, I think you will have to add an entry to the upgrade docs (i guess the user section???) about this breaking change and just mention how they go about fixing it (i.e. use the `DefaultGraphManager`). We'll probably also want to send a quick email to the dev list to just alert everyone to a break to make sure no one is buggin' about that. Finally, the JIRA issue will need the "breaking" label added to it. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1438) Consider GraphManager as an interface
[
https://issues.apache.org/jira/browse/TINKERPOP-1438?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905329#comment-15905329
]
ASF GitHub Bot commented on TINKERPOP-1438:
---
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/569
yeah - that's an obvious truth @pluradj . Normally we wouldn't allow such a
change on `tp32` because it does immediately make people's code not compile if
they used that class in that way but this class is fairly internal. If someone
is using it they likely know what they are doing. @dpitera I tend to agree that
this is outside the scope of what would need to remain compatible. That said, I
think you will have to add an entry to the upgrade docs (i guess the user
section???) about this breaking change and just mention how they go about
fixing it (i.e. use the `DefaultGraphManager`). We'll probably also want to
send a quick email to the dev list to just alert everyone to a break to make
sure no one is buggin' about that. Finally, the JIRA issue will need the
"breaking" label added to it.
> Consider GraphManager as an interface
> -
>
> Key: TINKERPOP-1438
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1438
> Project: TinkerPop
> Issue Type: Improvement
> Components: server
>Affects Versions: 3.2.2
>Reporter: stephen mallette
>Priority: Minor
> Labels: breaking
>
> If {{GraphManager}} were an interface it would make embedding Gremlin Server
> easier as {{Graph}} instances could be more easily supplied by the host
> application. In doing this, It also might be good to force a
> {{TraversalSource}} to be referred to by both the {{Graph}} name and
> {{TraversalSource}} name.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Updated] (TINKERPOP-1644) Improve script compilation process and include metrics
[ https://issues.apache.org/jira/browse/TINKERPOP-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] stephen mallette updated TINKERPOP-1644: Summary: Improve script compilation process and include metrics (was: Improve script compilation syncronisation) > Improve script compilation process and include metrics > -- > > Key: TINKERPOP-1644 > URL: https://issues.apache.org/jira/browse/TINKERPOP-1644 > Project: TinkerPop > Issue Type: Improvement > Components: groovy >Affects Versions: 3.2.4 >Reporter: Bryn Cooke >Assignee: stephen mallette > > Currently there is no synchronisation around script compilation. This means > that if a particularly heavy script is in use, many threads may end up > compiling the same script. > It would seem like a good idea to have some some sort of synchronisation to > prevent ever getting to this stage. > In addition, there will be cases where users will repeatedly submit broken > scripts to the server. In this case it is useful to log the error the first > time the script compilation is attempted and then cache the error for > subsequent runs. > Finally I have found some scripts take in excess of 30 seconds to compile. To > aid performance debugging the script compilation times should be included in > the logs. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (TINKERPOP-1644) Improve script compilation process and include metrics
[ https://issues.apache.org/jira/browse/TINKERPOP-1644?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905351#comment-15905351 ] ASF GitHub Bot commented on TINKERPOP-1644: --- GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/570 TINKERPOP-1644 Improve script compilation process and include metrics https://issues.apache.org/jira/browse/TINKERPOP-1644 This PR was started on #567 which was submitted by @BrynCooke. I've made some basic modifications and included various metrics about compilation that will be helpful in debugging things. Those metrics are reported up through Gremlin Server through the standard metric reporting system. Builds to success with `docker/build.sh -t -i -n`. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1644 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/570.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 #570 commit 13c93cabac51112a73f592e0fba12e515f643522 Author: BrynCooke Date: 2017-03-02T19:07:28Z TINKERPOP-1644 Improve script compilation syncronisation Script compilation is synchronised. Script compilation times are placed in to logs. Failed scripts will not be recompiled. Scripts that take over 5 seconds to compile are logged as a warning. commit deb68280d986b086120d56a73659b1869c07a475 Author: BrynCooke Date: 2017-03-07T16:54:58Z TINKERPOP-1644 Use future instead of maintaining a separate map of failures. commit 18778e405981523355319fa56bd04fd6cc07b845 Author: BrynCooke Date: 2017-03-08T12:24:46Z TINKERPOP-1644 Use Caffeine cache commit 17a72e0e1db6fe52d3a0821ef2bfae1eb39be856 Author: BrynCooke Date: 2017-03-08T13:23:30Z TINKERPOP-1644 Fix exception handling. commit 1df71c81e032daa9c9db6f626d99c39b37d434ce Author: BrynCooke Date: 2017-03-08T13:26:31Z TINKERPOP-1644 Fix exception handling. commit 608f024f7bb83eb168a6aa637d46ee0650674903 Author: Stephen Mallette Date: 2017-03-08T18:31:52Z TINKERPOP-1644 Remove gremlin-server caffeine dependency Caffeine is now down in gremlin-groovy. commit de1d58ab33c3121e9bafb5e4908f3e0d76c8e26e Author: Stephen Mallette Date: 2017-03-08T18:37:42Z TINKERPOP-1644 Minor code format updates commit 4bdeac4b796b38fb14d1be763e03cc837b57d3d7 Author: Stephen Mallette Date: 2017-03-08T21:13:06Z TINKERPOP-1644 Provided configuration options for GremlinGroovyScriptEngine Introduced new customizers to pass in configuration options to the GremlinGroovyScriptEngine. commit b29ba12109e7e88bc3465d08f6177e5ded17ca59 Author: Stephen Mallette Date: 2017-03-08T21:15:27Z TINKERPOP-1644 Updated changelog commit a06072b0502f9b42fee4c68dba554b4991406c36 Author: Stephen Mallette Date: 2017-03-08T21:17:40Z TINKERPOP-1644 Made the "counter" in GremlinGroovyScriptEngine static It seems that this field should be static and shared across all instances as the script name seems to share space with other GroovyClassLoaders. Not sure what would happen in the case of collision, but there doesn't seem to be much harm in ensuring better uniqueness. This counter wasn't used for tracking the number of scripts actually processed or anything so it should be ok to make this change without fear of breaking anything. commit 8ffa5af6ff56933c1955e88e8aa42c06cb69162b Author: Stephen Mallette Date: 2017-03-09T15:30:17Z TINKERPOP-1644 Added metrics to GremlinGroovyScriptEngine commit b689deb1dd1a0c745966a1ed5f8e7e3b2d543af2 Author: Stephen Mallette Date: 2017-03-09T17:25:35Z TINKERPOP-1644 Exposed GremlinScriptEngine metrics in Gremlin Server commit 9eb248ef0f0f29e8cf11cd5391ef8d993e37f7af Author: Stephen Mallette Date: 2017-03-09T18:15:04Z TINKERPOP-1644 Renamed metrics for GremlinScriptEngine instances. Included the name of the GremlinScriptEngine and prefixed each metric with GremlinServer class name to be consistent with other metrics. commit 37976526f1eac9fd06858f962f979aa98d3e5346 Author: Stephen Mallette Date: 2017-03-09T18:35:44Z TINKERPOP-1644 Docs for new metrics in Gremlin Server commit 33db1a3893c91a2dde100c8a0289312d4550503c Author: Stephen Mallette Date: 2017-03-09T19:51:36Z TINKERPOP-1644 Cleaned up a bad import > Improve script compilation process and include metrics > -- > > Key: TINKERPOP-1644 > URL: https://issues.apache.org/jira/browse/TINKERPOP-1644 > Project: TinkerPop > Issue Type: Improvement > Components: gr
[GitHub] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...
GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/570 TINKERPOP-1644 Improve script compilation process and include metrics https://issues.apache.org/jira/browse/TINKERPOP-1644 This PR was started on #567 which was submitted by @BrynCooke. I've made some basic modifications and included various metrics about compilation that will be helpful in debugging things. Those metrics are reported up through Gremlin Server through the standard metric reporting system. Builds to success with `docker/build.sh -t -i -n`. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1644 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/570.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 #570 commit 13c93cabac51112a73f592e0fba12e515f643522 Author: BrynCooke Date: 2017-03-02T19:07:28Z TINKERPOP-1644 Improve script compilation syncronisation Script compilation is synchronised. Script compilation times are placed in to logs. Failed scripts will not be recompiled. Scripts that take over 5 seconds to compile are logged as a warning. commit deb68280d986b086120d56a73659b1869c07a475 Author: BrynCooke Date: 2017-03-07T16:54:58Z TINKERPOP-1644 Use future instead of maintaining a separate map of failures. commit 18778e405981523355319fa56bd04fd6cc07b845 Author: BrynCooke Date: 2017-03-08T12:24:46Z TINKERPOP-1644 Use Caffeine cache commit 17a72e0e1db6fe52d3a0821ef2bfae1eb39be856 Author: BrynCooke Date: 2017-03-08T13:23:30Z TINKERPOP-1644 Fix exception handling. commit 1df71c81e032daa9c9db6f626d99c39b37d434ce Author: BrynCooke Date: 2017-03-08T13:26:31Z TINKERPOP-1644 Fix exception handling. commit 608f024f7bb83eb168a6aa637d46ee0650674903 Author: Stephen Mallette Date: 2017-03-08T18:31:52Z TINKERPOP-1644 Remove gremlin-server caffeine dependency Caffeine is now down in gremlin-groovy. commit de1d58ab33c3121e9bafb5e4908f3e0d76c8e26e Author: Stephen Mallette Date: 2017-03-08T18:37:42Z TINKERPOP-1644 Minor code format updates commit 4bdeac4b796b38fb14d1be763e03cc837b57d3d7 Author: Stephen Mallette Date: 2017-03-08T21:13:06Z TINKERPOP-1644 Provided configuration options for GremlinGroovyScriptEngine Introduced new customizers to pass in configuration options to the GremlinGroovyScriptEngine. commit b29ba12109e7e88bc3465d08f6177e5ded17ca59 Author: Stephen Mallette Date: 2017-03-08T21:15:27Z TINKERPOP-1644 Updated changelog commit a06072b0502f9b42fee4c68dba554b4991406c36 Author: Stephen Mallette Date: 2017-03-08T21:17:40Z TINKERPOP-1644 Made the "counter" in GremlinGroovyScriptEngine static It seems that this field should be static and shared across all instances as the script name seems to share space with other GroovyClassLoaders. Not sure what would happen in the case of collision, but there doesn't seem to be much harm in ensuring better uniqueness. This counter wasn't used for tracking the number of scripts actually processed or anything so it should be ok to make this change without fear of breaking anything. commit 8ffa5af6ff56933c1955e88e8aa42c06cb69162b Author: Stephen Mallette Date: 2017-03-09T15:30:17Z TINKERPOP-1644 Added metrics to GremlinGroovyScriptEngine commit b689deb1dd1a0c745966a1ed5f8e7e3b2d543af2 Author: Stephen Mallette Date: 2017-03-09T17:25:35Z TINKERPOP-1644 Exposed GremlinScriptEngine metrics in Gremlin Server commit 9eb248ef0f0f29e8cf11cd5391ef8d993e37f7af Author: Stephen Mallette Date: 2017-03-09T18:15:04Z TINKERPOP-1644 Renamed metrics for GremlinScriptEngine instances. Included the name of the GremlinScriptEngine and prefixed each metric with GremlinServer class name to be consistent with other metrics. commit 37976526f1eac9fd06858f962f979aa98d3e5346 Author: Stephen Mallette Date: 2017-03-09T18:35:44Z TINKERPOP-1644 Docs for new metrics in Gremlin Server commit 33db1a3893c91a2dde100c8a0289312d4550503c Author: Stephen Mallette Date: 2017-03-09T19:51:36Z TINKERPOP-1644 Cleaned up a bad import --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (TINKERPOP-1174) addVertex(Map properties) method
[
https://issues.apache.org/jira/browse/TINKERPOP-1174?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905609#comment-15905609
]
Jeremy Hanna commented on TINKERPOP-1174:
-
This could also apply to the {{addEdge}} method with the varargs versus a map.
> addVertex(Map properties) method
>
>
> Key: TINKERPOP-1174
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1174
> Project: TinkerPop
> Issue Type: Improvement
> Components: process
>Affects Versions: 3.1.1-incubating
>Reporter: Matthias Broecheler
>Assignee: stephen mallette
>
> which overloads the other {{addVertex(Object... properties)}} method with a
> map based version (of key-value pairs).
> Having this alternative to addVertex is nice because:
> 1) It allows you to submit a map when adding vertices on remote graphs via
> drivers. Right now, you have to convert the properties to an array which is
> awkward and unnecessary
> 2) It would work well with groovy's map based syntax so one can write:
> {{g.addVertex( key1 : "value1", key2: "value2")}}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (TINKERPOP-1174) addVertex(Map properties) method
[
https://issues.apache.org/jira/browse/TINKERPOP-1174?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15905622#comment-15905622
]
stephen mallette commented on TINKERPOP-1174:
-
There is also now the technical reason to look to implement this - scripts with
many parameters are more costly to compile than those with fewer, so this:
{code}
g.addV("x1",x1, "x2",x2, "x3",x3, .)
{code}
would compile significantly slower than:
{code}
m = ["x1":x1, "x2":x2, "x3":x3, .]
g.addV(m)
{code}
> addVertex(Map properties) method
>
>
> Key: TINKERPOP-1174
> URL: https://issues.apache.org/jira/browse/TINKERPOP-1174
> Project: TinkerPop
> Issue Type: Improvement
> Components: process
>Affects Versions: 3.1.1-incubating
>Reporter: Matthias Broecheler
>Assignee: stephen mallette
>
> which overloads the other {{addVertex(Object... properties)}} method with a
> map based version (of key-value pairs).
> Having this alternative to addVertex is nice because:
> 1) It allows you to submit a map when adding vertices on remote graphs via
> drivers. Right now, you have to convert the properties to an array which is
> awkward and unnecessary
> 2) It would work well with groovy's map based syntax so one can write:
> {{g.addVertex( key1 : "value1", key2: "value2")}}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[DISCUSS] GraphManager breaking change
This pull request is a good one and tackles an issue I've wanted to have done a while ago: https://github.com/apache/tinkerpop/pull/569 It makes GraphManager, a class in Gremlin Server that keeps track of hosted graphs, an interface rather than a concrete class, which would allow people embedding or extending Gremlin Server a bit more flexibility in how they supply graphs to Gremlin Server. Unfortunately, the GraphManager is a public class and changing to an interface means that anyone who instantiated GraphManager directly with its public constructor would end up with a break in their code on update to 3.2.5. We normally don't like to have that happen and would preserve such a change for 3.3.0 instead. In this case, however, I think that the given the nature of GraphManager this change is likely ok for 3.2.5 because: 1. GraphManager is an "internal" class and anyone using it likely knows what they are doing with it and can easily fix the breaking change (users will just need to swap GraphManager for DefaultGraphManager). 2. The chance of GraphManager being instantiated directly is pretty low. People using this class at all would normally use the instance provided by Gremlin Server itself and not create their own. Anyway, I just thought I would hang this out there for discussion in case anyone thought that having a breaking change in 3.2.5 of this sort would not be acceptable. I'll assume lazy consensus and proceed with shepherding this PR to the tp32 branch unless there are objections in the next 72 hours (Monday, March 13, 2016, 3:30PM EST).
