Hi, I wanted to give an update on the implementation of this solicit for some final feedback before it is completed. Based on the previous discussion, there was some desire to have the socket server available in 3.5.x or 3.6.x to provide additional testing of GLV's but there was also some apprehension as to how to undergo these changes without implementing breaking changes. I have opted to break the work into 2 parts, the first of which largely follows the previous suggestion of moving classes to a new module but leaving package names unchanged. As long as gremlin-driver maintains a dependency on these new modules, there should be no noticeable change to anyone who is using maven. I believe if someone is manually importing jars they might find themselves in a bit of trouble with this change if they don't include the new gremlin-util module. The second part of this refactor will introduce breaking changes, most notably changing the package names to reflect the new modules and removing/reducing dependencies where possible. The current idea with this structure is to target all of these changes for 3.7.x but if at some point in the future there is an increased desire to bring this into 3.5.x/3.6.x it should be relatively simple to backport the first part without needing to worry about the breaking changes.
There is an open PR here <https://github.com/apache/tinkerpop/pull/1850> for part one of these changes. I will follow this up soon with the planned breaking changes as well as adding at least one test per GLV using the socket server (and also something to process the config yaml for each GLV). While working on this I also noticed that while gremlin-server seems to always return results as a list, the socket server typically sends a single vertex on its own. I noticed that the dotnet graphson serializer assumes that results will be formatted in a list and will fail to deserialize the current socket server responses. I have not yet checked if this is true with other GLV's as well. My thoughts are to modify the socket server to always send results in a list to better match gremlin-server. Alternatively serializers could be patched to properly handle non-list results as well. I would greatly appreciate any comments or questions regarding the general approach or the choices made so far. Thanks, Cole Greer On Wed, Oct 26, 2022 at 4:57 AM Stephen Mallette <[email protected]> wrote: > My only concern with a focus on 3.7.x is that there are immediate problems > with 3.5.x that could use attention (and the capabilities of an extracted > SimpleWebSocketServer). I suppose it might be possible to work backward > from 3.7.x, i.e. make changes and test there, then backport something known > to work for 3.7.x to 3.6.x/3.5.x. Not sure if that is practical or not. Or > maybe for those earlier versions we have to just keep using the > SimpleWebSocketServer the way that we have been in Java only. > > Anyway, you laid out the options really nicely and I don't have better > suggestions so perhaps gremlin-utils in 3.7.x is the only reasonable > approach. > > > > On Tue, Oct 25, 2022 at 7:24 PM Cole Greer <[email protected] > .invalid> > wrote: > > > I believe that proceeding with option 1 is the best choice but it is > > not clear how it should be executed. As I see it there are 2 main > > questions that need answering. > > > > Is it better to move the serializers, message classes, etc. into > > gremlin-core or is it better to make a new module (perhaps called > > something like gremlin-utils) which is shared by gremlin-driver and > > gremlin-server? > > > > I believe that making a new module makes the most sense. Using > > gremlin-core for this purpose entered the discussion as a convenient > > option due to its place in the dependency tree but the more I consider > > it, the more I question if it makes sense to co-opt gremlin-core to > > fit this purpose. I suppose package org.apache.tinkerpop.gremlin.util > > already exists in gremlin-core so maybe this wouldn't be such a > > stretch to have gremlin-core encompass these additional classes. It > > really depends on what the community views as the intended purpose and > > scope of gremlin-core. If these classes fall within that scope then we > > should move these classes into core, otherwise we should proceed with > > a new module. > > > > Which version should these changes be targeted towards? > > > > The simplest approach is to target this solely to 3.7.x as there > it > > will be easy to justify minor breaks to users import lines. The > > question is what is the value of having these changes in 3.5.x and how > > could this be implemented without breaking compatibility? I see 3 > > approaches to forcing these changes into 3.5.x: > > > > a. As previously mentioned we could move the classes to a > > different > > module but leave the package lines untouched. I've tried this approach > > and it seems to work without issue but I agree that it just feels > > wrong to have a driver package outside of the driver module. > > > > b. Leave the original classes untouched in gremlin-driver > > (potentially marked as deprecated) and duplicate them to the new > > module/core. Original classes could then be removed in 3.7. This could > > cause problems if the ClassLoader views these as different and > > incompatible types. Also feels like this would be irritating to > > maintain in the short term. Any changes to one file would need to be > > duplicated as well. > > > > c. Same as b but instead of leaving the full class > behind, > > leave > > some sort wrapper or subclass in the driver. Shares the same issues as > > b except for code duplication. > > > > None of these strike me as good choices but a. seems the least > > problematic. I'm not sure how badly we want to have SimpleSocketServer > > or any derivative test servers available to all GLV's in 3.5.x. I > > would like to use it if possible for some of the work I'm doing > > towards TINKERPOP-2480 but I could always make do without it or delay > > support in all GLV's for 3.7. Unless there is a strong desire to make > > the test server available in 3.5.x it is probably best to leave it for > > 3.7. > > > > Overall I think it is best to make a new module for these classes and > > not to include it until 3.7. I do believe there are still some merits > > to bringing improved testing to the GLV's in 3.5.x and 3.6.x but I am > > unsure if this is worth dealing with potentially breaking changes and > > funky package names. > > > > Cole Greer > > > > > > On Tue, Oct 25, 2022 at 3:42 AM Stephen Mallette <[email protected]> > > wrote: > > > > > I've considered Option 1 more than once. Perhaps it is just time to do > > it. > > > I'd be in favor of that, however it would have to be done for 3.7.x as > it > > > does represent a breaking change in a sense. > > > > > > Or maybe it isn't a breaking change if you don't change the package > > names? > > > gremlin-driver already depends on gremlin-core so maybe it doesn't > > matter? > > > so, for 3.5.x you just move stuff and for 3.7.x I think we'd want to > > rename > > > the package - "driver" doesn't seem suitable for gremlin-core to me for > > > some reason. thoughts? > > > > > > > > > On Mon, Oct 24, 2022 at 12:20 PM Cole Greer <[email protected] > > > .invalid> > > > wrote: > > > > > > > Hi Stephen, > > > > > > > > I was hoping to get some additional feedback regarding the pitch I > made > > > to > > > > make the SimpleSocketServer available to all the GLV's. I agree with > > your > > > > suggestion to create a new module gremlin-tools/gremlin-socket-server > > as > > > it > > > > seems like a good first step towards some sort of universal test > > server. > > > > Unfortunately it doesn't immediately resolve any dependency issues. > > > > TestWSGremlinInitializer which I would want to include in this new > > module > > > > depends on several components of gremlin-driver including > > RequestMessage, > > > > ResponseMessage, and some of the serializers. This is precisely the > > > reason > > > > that gremlin-server has a dependence on the driver as well and > creates > > a > > > > cyclic dependency problem between the driver and the new test server > > > > module. After discussions with the BitQuill team we see 3 options: > > > > > > > > 1. Refactor gremlin-driver and extract all classes which are needed > by > > > the > > > > server into gremlin-core or some other new module. By moving all of: > > > > > > > > tinkerpop.gremlin.driver.message.* > > > > tinkerpop.gremlin.driver.ser.* > > > > tinkerpop.gremlin.driver.MessageSerializer > > > > tinkerpop.gremlin.driver.Tokens > > > > > > > > into a new package, the gremlin-tools/gremlin-socket-server will have > > > > access to all of the classes it needs without a dependency on > > > > gremlin-driver, resolving the cyclic dependency issue. Furthermore > this > > > > change would allow us to reduce gremlin-server's dependency on > > > > gremlin-driver to a test scope which I believe is an improvement. > > > > Pros: Most robust and long term solution. Allows the extraction > of > > > > SimpleSocketServer from gremlin-driver without losing and > functionality > > > or > > > > ability for future extension. Allows all GLV's to depend on the test > > > server > > > > without a dependence on gremlin-driver. Provides a good foundation to > > > build > > > > a universal test server for all the GLV's. > > > > Cons: Represents a huge structural change to gremlin-driver. > Would > > > need > > > > a lot of consideration in how to handle this move to minimize user > > > impact. > > > > 2. Create a docker image from the simple-socket-server as it > currently > > > > exists as part of the java driver. Publish this image to dockerhub, > and > > > > configure GLV’s to run tests against this image. > > > > Pros: Allows the socket server to continue with all its current > > > > features and be accessible to all GLV’s without creating a direct > > > > dependency on the java driver. No significant changes required to > java > > > > driver. > > > > Cons: Having a published test image represents a barrier to > > changing > > > > and adding new functionality or testing behaviours to the > > socket-server. > > > > > > > > 3. Extract Simple-Socket-Server to a new module and remove all > > > dependencies > > > > on the java driver. Dependence on serializers and ResponseMessage > > classes > > > > could be stripped out in favour of hardcoded response strings. As it > > > stands > > > > right now the socket server is only used to test for a small handful > of > > > > scenarios and it would be fairly straightforward to have a few > > hardcoded > > > > responses to select from. > > > > Pros: Completely removes dependency on java driver without any > > large > > > or > > > > breaking changes. > > > > Cons: Limits the ability to grow and extend the current socket > > > server. > > > > Not a useful step towards the universal test server which Stephen has > > > been > > > > talking about. > > > > > > > > Of all of these my preference is to go with number 1. I believe it > > would > > > be > > > > a good improvement to the structure of tinkerpop, but I'm not sure > how > > > best > > > > to handle the migration. 2 and 3 both represent shorter term > solutions > > to > > > > get some use of SimpleSocketServer in the GLV's but don't make the > same > > > > progress towards standardizing the testing between all the GLV's. I > > > suppose > > > > a 4th option is to shelve all of this for now and proceed with > > > maintaining > > > > independent connection tests for all the GLV's but my preference > would > > be > > > > to try and improve on this where possible. > > > > > > > > Thanks, > > > > > > > > > > > > *Cole Greer* > > > > > > > > On Mon, Oct 17, 2022 at 5:03 AM Stephen Mallette < > [email protected] > > > > > > > wrote: > > > > > > > > > If you think it's helpful, I'd say that's fine. Are you suggesting > it > > > be > > > > > moved from gremlin-driver/src/test to gremlin-server/src/main? > maybe > > it > > > > > would go best in a new /server/util/test package? > > > > > > > > > > The only problem with moving it is that you end up with a circular > > > > > relationship. gremlin-server depends on gremlin-driver so if you > move > > > it > > > > > the gremlin-driver tests that depend on it won't work anymore. > It's a > > > bit > > > > > weird but maybe you keep it in gremlin-driver and just move it to > > > > > gremlin-driver/.../util/test? > > > > > > > > > > Or I guess the driver tests could move to gremlin-server along with > > the > > > > > SimpleSocketServer related classes? that's a bit weird too though. > > > > > > > > > > Or maybe a new module is on order? create > > > > > gremlin-tools/gremlin-socket-server? i think you'd avoid dependency > > > > issues > > > > > there between gremlin-driver and gremlin-server and the GLVs. Maybe > > > this > > > > is > > > > > the best way as it would allow the independent development of a > > really > > > > > robust test server without muddying existing packages with test > > > > components. > > > > > > > > > > Sorry, I just sorta rambled my way to that last point which seems > > like > > > it > > > > > could be the best one. > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Oct 14, 2022 at 4:27 PM Cole Greer <[email protected] > > > > > .invalid> > > > > > wrote: > > > > > > > > > > > Hi everyone, > > > > > > > > > > > > I have been working on TINKERPOP-2480 according to Ken's recent > > > > > > proposal ( > > > > > https://lists.apache.org/thread/x8lt6mk04r7h0nsdy9q3pz1pbo1t28tj > > > > > > ). > > > > > > I have found that the SimpleSocketServer combined with > > > > > > TestWSGremlinInitializer in Gremlin-Driver to be a very useful > > > > > > resource for creating tests which require custom response > behaviour > > > > > > from the server. From what I have seen this is something which > the > > > > > > other GLV's are currently lacking. I believe having such a test > > > server > > > > > > accessible to the other GLV's would be a useful step towards > > > > > > standardizing the testing between all the GLV's and bringing more > > > > > > behavioural tests to the GLV's. In order to do this I would > propose > > > > > > moving the SimpleSocketServer from the tinkerpop.gremlin.driver > > > > > > package to tinkerpop.gremlin.server and running it in the > existing > > > > > > gremlin-server-test docker container on a new port. > > > > > > > > > > > > I would greatly appreciate any feedback on this idea, whether you > > > find > > > > > > it to be useful or have any concerns. > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Cole > > > > > > > > > > > > > > > > > > > > >
