You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by spmallette <gi...@git.apache.org> on 2017/03/10 16:19:00 UTC

[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 <br...@gmail.com>
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 <br...@gmail.com>
Date:   2017-03-07T16:54:58Z

    TINKERPOP-1644
    Use future instead of maintaining a separate map of failures.

commit 18778e405981523355319fa56bd04fd6cc07b845
Author: BrynCooke <br...@gmail.com>
Date:   2017-03-08T12:24:46Z

    TINKERPOP-1644
    Use Caffeine cache

commit 17a72e0e1db6fe52d3a0821ef2bfae1eb39be856
Author: BrynCooke <br...@gmail.com>
Date:   2017-03-08T13:23:30Z

    TINKERPOP-1644
    Fix exception handling.

commit 1df71c81e032daa9c9db6f626d99c39b37d434ce
Author: BrynCooke <br...@gmail.com>
Date:   2017-03-08T13:26:31Z

    TINKERPOP-1644
    Fix exception handling.

commit 608f024f7bb83eb168a6aa637d46ee0650674903
Author: Stephen Mallette <sp...@genoprime.com>
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 <sp...@genoprime.com>
Date:   2017-03-08T18:37:42Z

    TINKERPOP-1644 Minor code format updates

commit 4bdeac4b796b38fb14d1be763e03cc837b57d3d7
Author: Stephen Mallette <sp...@genoprime.com>
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 <sp...@genoprime.com>
Date:   2017-03-08T21:15:27Z

    TINKERPOP-1644 Updated changelog

commit a06072b0502f9b42fee4c68dba554b4991406c36
Author: Stephen Mallette <sp...@genoprime.com>
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 <sp...@genoprime.com>
Date:   2017-03-09T15:30:17Z

    TINKERPOP-1644 Added metrics to GremlinGroovyScriptEngine

commit b689deb1dd1a0c745966a1ed5f8e7e3b2d543af2
Author: Stephen Mallette <sp...@genoprime.com>
Date:   2017-03-09T17:25:35Z

    TINKERPOP-1644 Exposed GremlinScriptEngine metrics in Gremlin Server

commit 9eb248ef0f0f29e8cf11cd5391ef8d993e37f7af
Author: Stephen Mallette <sp...@genoprime.com>
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 <sp...@genoprime.com>
Date:   2017-03-09T18:35:44Z

    TINKERPOP-1644 Docs for new metrics in Gremlin Server

commit 33db1a3893c91a2dde100c8a0289312d4550503c
Author: Stephen Mallette <sp...@genoprime.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop issue #570: TINKERPOP-1644 Improve script compilation process and ...

Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:

    https://github.com/apache/tinkerpop/pull/570
  
    Should work if you use the `CompilationOptionsCustomizerProvider` in your gremlin-server.yaml as you tried to do with the `CompilationOptionsCustomizer` previously:
    
    ```text
    "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider":[10],
    ```
    
    I just pushed another commit with a few lines of additional docs related to that configuration option.


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105747173
  
    --- Diff: docs/src/reference/gremlin-applications.asciidoc ---
    @@ -1560,6 +1560,11 @@ and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th a
     * `op.traversal` - the number of `Traveral` executions, mean rate, 1, 5, and 15 minute rates, minimum, maximum, median,
     mean, and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th and 99.9th percentile evaluation
     times.
    +* `engine-name.session.session-id.*` - metrics related to different `GremlinScriptEngine` instanc configured for
    +session-based requests where "engine-name" will be the actual name of the engine, such as "gremlin-groovy" and
    +"session-id" will be the identifier for the session itself.
    +* `engine-name.sessionless.*` - metrics related to different `GremlinScriptEngine` instanc configured for sessionless
    --- End diff --
    
    instances


---
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] tinkerpop issue #570: TINKERPOP-1644 Improve script compilation process and ...

Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:

    https://github.com/apache/tinkerpop/pull/570
  
    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] tinkerpop issue #570: TINKERPOP-1644 Improve script compilation process and ...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:

    https://github.com/apache/tinkerpop/pull/570
  
    @spmallette If you could just clarify if compilation timeout is configurable in 3.2? if so, how?


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105750603
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java ---
    @@ -149,19 +159,64 @@
             }
         };
     
    +    private GremlinGroovyClassLoader loader;
    +
         /**
          * Script to generated Class map.
          */
    -    private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
    +    private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder().
    +            softValues().
    +            recordStats().
    +            build(new CacheLoader<String, Future<Class>>() {
    +        @Override
    +        public Future<Class> load(final String script) throws Exception {
    +            final long start = System.currentTimeMillis();
    +
    +            return CompletableFuture.supplyAsync(() -> {
    +                try {
    +                    return loader.parseClass(script, generateScriptName());
    +                } catch (CompilationFailedException e) {
    +                    final long finish = System.currentTimeMillis();
    +                    log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e);
    +                    failedCompilationCount.incrementAndGet();
    --- End diff --
    
    I'm not sure how to trigger this. I can't get `gremlin-groovy.sessionless.class-cache.load-failure-count` to increment.
    Tried
    `curl "http://localhost:8182?gremlin=g.V().beavis()"`
    and
    ```
    :remote connect tinkerpop.server conf/remote.yaml
    :> g.V().beavis()
    ```



---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105754194
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java ---
    @@ -149,19 +159,64 @@
             }
         };
     
    +    private GremlinGroovyClassLoader loader;
    +
         /**
          * Script to generated Class map.
          */
    -    private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
    +    private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder().
    +            softValues().
    +            recordStats().
    +            build(new CacheLoader<String, Future<Class>>() {
    +        @Override
    +        public Future<Class> load(final String script) throws Exception {
    +            final long start = System.currentTimeMillis();
    +
    +            return CompletableFuture.supplyAsync(() -> {
    +                try {
    +                    return loader.parseClass(script, generateScriptName());
    +                } catch (CompilationFailedException e) {
    +                    final long finish = System.currentTimeMillis();
    +                    log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e);
    +                    failedCompilationCount.incrementAndGet();
    --- End diff --
    
    This is a syntactically correct script. Did you try something like `g.V().butthead)(` or a huge script that runs into a compilation timeout?


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105959819
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java ---
    @@ -31,13 +31,17 @@
     @Deprecated
     public class CompilationOptionsCustomizerProvider implements CompilerCustomizerProvider {
     
    -    private final int expectedCompilationTime;
    +    private final long expectedCompilationTime;
     
    -    public CompilationOptionsCustomizerProvider(final int expectedCompilationTime) {
    -        this.expectedCompilationTime = expectedCompilationTime;
    +    public CompilationOptionsCustomizerProvider(final Integer expectedCompilationTime) {
    +        this.expectedCompilationTime = expectedCompilationTime.longValue();
         }
     
    -    public int getExpectedCompilationTime() {
    +    public CompilationOptionsCustomizerProvider(final Long expectedCompilationTime) {
    +        this.expectedCompilationTime = expectedCompilationTime.intValue();
    --- End diff --
    
    Did you mean `.longValue()`?


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105952425
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java ---
    @@ -0,0 +1,39 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.tinkerpop.gremlin.groovy.jsr223;
    +
    +import org.apache.tinkerpop.gremlin.jsr223.Customizer;
    +
    +/**
    + * Provides some custom compilation options to the {@link GremlinGroovyScriptEngine}.
    + *
    + * @author Stephen Mallette (http://stephen.genoprime.com)
    + */
    +class CompilationOptionsCustomizer implements Customizer {
    +
    +    private final int expectedCompilationTime;
    +
    +    public CompilationOptionsCustomizer(final int expectedCompilationTime) {
    --- End diff --
    
    I'm converting the integers to longs just to be consistent with other times being in long milliseconds. The new plugin system doesn't work in the same way as the old so trying to push a `CompilationOptionsCustomizer` into the list of `compilerCustomizerProviders` won't work.  I don't really think we should promote the new plugin system until 3.3.0, but if you really want to give it a shot you'll need to look at the SNAPSHOT docs and revised config files for gremlin-server on the master branch that show how these new configurations go in place. You can get an idea of how this will work under this new model here:
    
    https://github.com/apache/tinkerpop/blob/master/gremlin-server/conf/gremlin-server-secure.yaml#L36
    
    You would basically add a config option to that line for `expectedCompilationTime` and it will configure the `CompilationOptionsCustomizer` into the `GremlinScriptEngine`.
    
    
    When I go to merge this to master, I'll need to update docs to reflect how this configuration setting works. I will comment back here as soon as I've pushed the latest changed.


---
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] tinkerpop issue #570: TINKERPOP-1644 Improve script compilation process and ...

Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:

    https://github.com/apache/tinkerpop/pull/570
  
    Changes have been pushed - hopefully I have all issues addressed 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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105959185
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java ---
    @@ -0,0 +1,39 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.tinkerpop.gremlin.groovy.jsr223;
    +
    +import org.apache.tinkerpop.gremlin.jsr223.Customizer;
    +
    +/**
    + * Provides some custom compilation options to the {@link GremlinGroovyScriptEngine}.
    + *
    + * @author Stephen Mallette (http://stephen.genoprime.com)
    + */
    +class CompilationOptionsCustomizer implements Customizer {
    +
    +    private final int expectedCompilationTime;
    +
    +    public CompilationOptionsCustomizer(final int expectedCompilationTime) {
    --- End diff --
    
    Ok. I'm just trying to figure out how to test the compilation timeout without having a 5-second script.  I would like to set the timeout to 1ms.  Even using the deprecated config doesn't seem to work for me. 
    ```
        compilerCustomizerProviders: {
              "org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider": [1]}
    ```
    Error:
    ```
    [WARN] ScriptEngines - Could not instantiate CompilerCustomizerProvider implementation [org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.CompilationOptionsCustomizerProvider].  It will not be applied.
    java.lang.IllegalStateException: Could not configure org.apache.tinkerpop.gremlin.groovy.jsr223.customizer.ConfigurationCustomizerProvider with the supplied options [1]
    	at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.lambda$createScriptEngine$15(ScriptEngines.java:418)
    	at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684)
    	at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.createScriptEngine(ScriptEngines.java:399)
    	at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.reload(ScriptEngines.java:187)
    	at org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$createScriptEngines$13(GremlinExecutor.java:512)
    ```


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105747128
  
    --- Diff: docs/src/reference/gremlin-applications.asciidoc ---
    @@ -1560,6 +1560,11 @@ and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th a
     * `op.traversal` - the number of `Traveral` executions, mean rate, 1, 5, and 15 minute rates, minimum, maximum, median,
     mean, and standard deviation evaluation times, as well as the 75th, 95th, 98th, 99th and 99.9th percentile evaluation
     times.
    +* `engine-name.session.session-id.*` - metrics related to different `GremlinScriptEngine` instanc configured for
    --- End diff --
    
    instances


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105759976
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java ---
    @@ -149,19 +159,64 @@
             }
         };
     
    +    private GremlinGroovyClassLoader loader;
    +
         /**
          * Script to generated Class map.
          */
    -    private ManagedConcurrentValueMap<String, Class> classMap = new ManagedConcurrentValueMap<>(ReferenceBundle.getSoftBundle());
    +    private final LoadingCache<String, Future<Class>> classMap = Caffeine.newBuilder().
    +            softValues().
    +            recordStats().
    +            build(new CacheLoader<String, Future<Class>>() {
    +        @Override
    +        public Future<Class> load(final String script) throws Exception {
    +            final long start = System.currentTimeMillis();
    +
    +            return CompletableFuture.supplyAsync(() -> {
    +                try {
    +                    return loader.parseClass(script, generateScriptName());
    +                } catch (CompilationFailedException e) {
    +                    final long finish = System.currentTimeMillis();
    +                    log.error("Script compilation FAILED {} took {}ms {}", script, finish - start, e);
    +                    failedCompilationCount.incrementAndGet();
    --- End diff --
    
    That does the trick.  Making a separate comment about compilation timeout...



---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/tinkerpop/pull/570


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105764358
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/CompilationOptionsCustomizer.java ---
    @@ -0,0 +1,39 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package org.apache.tinkerpop.gremlin.groovy.jsr223;
    +
    +import org.apache.tinkerpop.gremlin.jsr223.Customizer;
    +
    +/**
    + * Provides some custom compilation options to the {@link GremlinGroovyScriptEngine}.
    + *
    + * @author Stephen Mallette (http://stephen.genoprime.com)
    + */
    +class CompilationOptionsCustomizer implements Customizer {
    +
    +    private final int expectedCompilationTime;
    +
    +    public CompilationOptionsCustomizer(final int expectedCompilationTime) {
    --- End diff --
    
    I believe `int` should be `Integer` looking at the other customizers.  However, there seems to be issues with customizers in general where in 3.2.5 they are deprecated and no longer public classes. So when I modify this class with Integer, (or try to use the newer, non 'customizer' package customizers) I get the following error.
    
    ```
          compilerCustomizerProviders: {
                  "org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer":[1]}
    ```
    Error:
    ```
    [WARN] ScriptEngines - Could not instantiate CompilerCustomizerProvider implementation [org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer].  It will not be applied.
    java.lang.IllegalAccessException: Class org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines can not access a member of class org.apache.tinkerpop.gremlin.groovy.jsr223.CompilationOptionsCustomizer with modifiers "public"
    	at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
    	at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
    	at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
    	at java.lang.reflect.Constructor.newInstance(Constructor.java:413)
    	at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.lambda$createScriptEngine$15(ScriptEngines.java:416)
    	at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684)
    	at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.createScriptEngine(ScriptEngines.java:399)
    	at org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.reload(ScriptEngines.java:187)
    ```


---
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] tinkerpop pull request #570: TINKERPOP-1644 Improve script compilation proce...

Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/570#discussion_r105962373
  
    --- Diff: gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/customizer/CompilationOptionsCustomizerProvider.java ---
    @@ -31,13 +31,17 @@
     @Deprecated
     public class CompilationOptionsCustomizerProvider implements CompilerCustomizerProvider {
     
    -    private final int expectedCompilationTime;
    +    private final long expectedCompilationTime;
     
    -    public CompilationOptionsCustomizerProvider(final int expectedCompilationTime) {
    -        this.expectedCompilationTime = expectedCompilationTime;
    +    public CompilationOptionsCustomizerProvider(final Integer expectedCompilationTime) {
    +        this.expectedCompilationTime = expectedCompilationTime.longValue();
         }
     
    -    public int getExpectedCompilationTime() {
    +    public CompilationOptionsCustomizerProvider(final Long expectedCompilationTime) {
    +        this.expectedCompilationTime = expectedCompilationTime.intValue();
    --- End diff --
    
    ffs


---
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] tinkerpop issue #570: TINKERPOP-1644 Improve script compilation process and ...

Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:

    https://github.com/apache/tinkerpop/pull/570
  
    Works now. 
    
    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.
---