[
https://issues.apache.org/jira/browse/TINKERPOP-2436?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17213893#comment-17213893
]
ASF GitHub Bot commented on TINKERPOP-2436:
-------------------------------------------
spmallette edited a comment on pull request #1342:
URL: https://github.com/apache/tinkerpop/pull/1342#issuecomment-708385238
No comments from anyone so I suppose there are no high-priority concerns. I
suppose this isn't a bad change and might actually help new users who might get
their configurations wrong and then wonder why they are getting errors. I
suppose that has happened before. Here's some things that I think need to be
done to get this one ready for proper review:
1. I would prefer that `DefaultGraphManager` be left alone and instead make
it a basis for extension. I would recommend creating an extension of
`DefaultGraphManager` called `CheckedGraphManager` (open to another name if you
have a suggestion) that basically do the functionality you are suggesting. I
would think this new class would just call `super()` in the constructor then do
the validation. You will need to alter `DefaultGraphManager` scopes to make
this work.
1. I think we should continue to target `master` (3.5.0) with this change
and therefore it is safe to change the default
[here](https://github.com/apache/tinkerpop/blob/master/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java#L192)
to your new class.
1. A unit test would go in this package
[here](https://github.com/apache/tinkerpop/blob/501e926afd5dd956b2c2c823c5304978f732d54f/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/util/)
1. A more robust error message
[here](https://github.com/apache/tinkerpop/pull/1342/files#diff-6c52c03578764d296f1f68e7d7a7bca7c34ee8af013737f596b9879afa730a08R73)
would be nice and I think we'd prefer `IllegalStateException`
1. For situations where there are no graphs because of exceptions
[here](https://github.com/apache/tinkerpop/pull/1342/files#diff-6c52c03578764d296f1f68e7d7a7bca7c34ee8af013737f596b9879afa730a08R74-R79)
I'm not sure we need to rethrow the exceptions as we all ready WARN above of
such problems. I think a simple well-formed `IllegalStateException` that no
graphs are present and the server stopping should be sufficient.
1. I presume throwing an exception will stop Gremlin Server from starting. I
don't think that breaks any of our integration tests because they all start
with configured graphs. I suppose you will find that out. We might want
integration tests for this change to ensure the server shuts down
nicely/cleanly, but let's see what the rest of these changes look like first.
1. This is a breaking change in behavior so I think we will need [Upgrade
Documentation](https://github.com/apache/tinkerpop/blob/501e926afd5dd956b2c2c823c5304978f732d54f/docs/src/upgrade/release-3.5.x.asciidoc)
for both users and providers on this PR.
I'm not sure if that's everything or not, but I can't see much further on
this one without some of these changes in place. It's always interesting how
software works - take something though to be "simple" and it quickly unravels
into something quite the opposite. Please let me know if you have any questions
@mmadoo
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> The gremlin server starts even if all graphs instantiation has failed
> ---------------------------------------------------------------------
>
> Key: TINKERPOP-2436
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2436
> Project: TinkerPop
> Issue Type: Bug
> Components: server
> Affects Versions: 3.4.8
> Reporter: Nicolas Trangosi
> Priority: Major
>
> Sometimes the gremlin server fails to open the graph due to backend failure.
> In this case, gremlin server starts even if could not serve any request as
> gremlin-groovy GremlinScriptEngine is not initialized. I think that in this
> case, gremlin server should stop (and be restarted in a kubernates cluster)
> {{8 Oct 2020 12:35:51,923 8660 [gremlin-server-exec-1] ERROR
> org.apache.tinkerpop.gremlin.jsr223.DefaultGremlinScriptEngineManager -
> Could not create GremlinScriptEngine for gremlin-groovy}}
> {{ java.lang.IllegalStateException: javax.script.ScriptException:
> javax.script.ScriptException: groovy.lang.MissingPropertyException: No such
> property: graph for class: Script1 }}
> {noformat}
> 08 Oct 2020 12:35:45,228 1965 [main] WARN
> org.apache.tinkerpop.gremlin.server.GremlinServer - Graph [graph] configured
> at [/etc/opt/janusgraph/janusgraph.properties] could not be instantiated and
> will not be available in Gremlin Server. GraphFactory message: GraphFactory
> could not instantiate this Graph implementation [class
> org.janusgraph.core.JanusGraphFactory]
> 08 Oct 2020 12:35:45,228 1965 [main] WARN
> org.apache.tinkerpop.gremlin.server.GremlinServer - Graph [graph] configured
> at [/etc/opt/janusgraph/janusgraph.properties] could not be instantiated and
> will not be available in Gremlin Server. GraphFactory message: GraphFactory
> could not instantiate this Graph implementation [class
> org.janusgraph.core.JanusGraphFactory]
> java.lang.RuntimeException: GraphFactory could not instantiate this Graph
> implementation [class org.janusgraph.core.JanusGraphFactory]
> at
> org.apache.tinkerpop.gremlin.structure.util.GraphFactory.open(GraphFactory.java:81)
> at
> org.apache.tinkerpop.gremlin.structure.util.GraphFactory.open(GraphFactory.java:69)
> at ...
> {noformat}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
