So, I was taking a closer look at this there's potentially a larger mess we want to address.
TLDR: instead of removing groovy-sql, replace all groovy deps with groovy-all-indy. First, a bit of history. tp32 did actually use groovy.sql.Sql in ConsoleImportCustomizerProvider. This class was deprecated in 3.2.4. Later, it was removed in 3.3.0. With the plan being to remove groovy-sql completely in 3.4.0, I was testing my instructions on using the :install feature to reinstall it just in case had someone depended on it. It of course downloaded groovy-sql but it was different than what was in lib/. It was missing '-indy'. It also downloaded the non-indy version of groovy. I had to go learn what '-indy' was about. For those who don't know '-indy' is a maven classifier to denote that the jar was built with Java 1.7 'invokedymanic' support as opposed the legacy 'call site'. The short of it is that invokedymanic is more performant than call site. As I was comparing groovy lib/, I noticed that console and server already include both groovy-2.4.15-indy.jar and groovy-2.4.15.jar. However, the docs[1] state that only one or the other should be included. Same goes for any 'indy' and 'non-indy' jar of the same module. Luckily, we don't have any other conflict like that although there is a mix of other indy non-indy modules. I tried again and was unable to download the indy version of groovy-sql because the :install command does not allow a 'classifier' parameter. It will only download the non-indy verison of modules. Even if it could download the 'indy' version, the groovy dependencies are built such that all 'indy' modules have a dependency on non-indy modules. So, there are several issues: 1) there is included a conflicting mix of indy and non-indy builds of the same module - groovy. This is bad and should be fixed. 2) there is included a mix of indy and non-indy builds of different modules. This is not necessarily bad but does mean we're using potentially less performant pieces of groovy. indy) groovysh, json, jsr223 non-indy) console, swing, templates, xml 3) Install command doesn't support classifier parameter. I'm not aware of anyone complaining about this before so it's not necessarily an issue in itself. However, if we were to desire that all groovy modules be indy-only, then classifier support would be required. But then I don't know how to prevent the non-indy dependencies of that module from being downloaded. It seems like a mix would always occur. There is one simple solution interestingly enough: depend only on groovy-all-indy. Not only does this keep all indy modules consistent and prevent non-indy conflicts/duplicates, it actually shrinks the distribution size by 4M. This is from not distributing the duplicate non-indy version of groovy and because groovy-all isn't much bigger than the separate modules we have now. The other benefit is that we can reduce groovy dependencies down to a single module across the project. The alternative is ugly - having a direct dependency on each module in the dependency tree and having an excludes on each of those dependencies to prevent them from loading the non-indy deps. In conclusion, instead of removing groovy-sql, I am proposing that instead we replace all groovy deps with groovy-all-indy. Thoughts? Stephen, are the groovy scripts submitted to Gremlin Server compiled with invokedynamic? What about Gremlin Console? I know the groovy console does not have this enabled by default. 1. http://docs.groovy-lang.org/docs/groovy-2.4.15/html/documentation/invokedynamic-support.html#_two_jars Robert Dale On Fri, Sep 14, 2018 at 9:32 AM Robert Dale <[email protected]> wrote: > No, nothing in the docs. There are some references to java.sql.Timestamp > but only within the context of gryo. I'll just make an upgrade note that > it was removed and if required, it can be installed through the :install > command. > > Robert Dale > > > On Fri, Sep 14, 2018 at 6:34 AM Stephen Mallette <[email protected]> > wrote: > >> I think groovy-sql was there because there was a time when I was into >> polyglot data transforms in Gremlin and groovy-sql enabled you to connect >> to JDBC data sources in a nice way. So, I included groovy-sql as a >> convenience. I'm not against removing it, though I think we should remove >> it in 3.4.0 only in case someone is currently depending on it indirectly. >> Please try to double check that we aren't using groovy-sql in any of the >> documentation for some odd example or something. >> >> On Thu, Sep 13, 2018 at 5:20 PM Robert Dale <[email protected]> wrote: >> >> > gremlin-driver has a direct dependency on groovy-sql. I removed it in >> > master and all tests pass (docker/build.sh -i -t -n) so there does not >> > appear to be any direct or indirect usage. >> > >> > Why is it there? Looks like it was pulled in because of >> > https://issues.apache.org/jira/browse/TINKERPOP-713 >> > However, groovy-sql does not appear to be a dependency of >> groovy-console or >> > anything groovy (except groovy-all). Wasn't then, isn't now. >> > >> > Any objections to removing it? >> > >> > -- >> > Robert Dale >> > >> >
