TINKERPOP-2246 Use ResponseHandlerContext to ensure single final response to client

[divijvaidya]

https://issues.apache.org/jira/browse/TINKERPOP-2246

This code change consolidates the usage of ResponseHandlerContext for sending out response to the client.

Testing
gremlin-server: mvn clean install -DskipIntegrationTests=false
gremlin-driver: mvn clean install -DskipIntegrationTests=false

[spmallette]

I think this was a good change, however now that I look at it I think it was partially implemented the way that it was to avoid a breaking change in Gremlin Server along 3.3.x and 3.4.x. Is there a way to approach this with an eye for deprecation?

Here's some ideas I'm just throwing out there to consider.......they aren't fully thought through and just meant to foster some discussion.

Maybe OpProcessor could deprecate ThrowingConsumer<Context> select(final Context ctx) and introduce the ResponseHandlerContext version of that method which would have a default implementation of returning null (or maybe a static do-nothing marker ThrowingConsumer instance). Then in OpSelectorHandler we would just try that method first and if it returns null we just use the old deprecated method. I guess that would mean that OpExecutorHandler will need some further adjustment to handle both types of Pairs coming at it....maybe that's not so good.

What about Context? Do we need both a ResponseHandlerContext and a Context? Now that I'm looking at it, I kinda wonder if we couldn't just merge the functionality of ResponseHandlerContext into Context and then deprecate ResponseHandlerContext and related methods? I did speak with @dimas-b for a bit and he didn't see any problem with merging the two - his reasoning for introducing ResponseHandlerContext was mostly to not alter what Context was basically doing as a simple holder for Gremlin Server objects as ResponseHandlerContext was bringing in actions like writeAndFlush(). Maybe this is an avenue to explore?

Other ideas?

[divijvaidya]

I apologize for the delay on this PR. I will get back to it in the next few days.

[divijvaidya]

I have merged the ResponseHandlerContext into Context. There is no change in the interfaces and hence, code is backward compatible. This change also brings this closer to how Netty works with the context, where the pipeline operations like write and flush are done via the context object.

Let me know if this addresses your concerns.

[spmallette]

Wow - that looks a lot better. Great job! Did you have to completely delete ResponseHandlerContext though? It seems like there was a way to deprecate it while maintaining the method signatures that were using it thus avoiding a breaking change. Was there a problem that forced you into complete removal?

[divijvaidya]

Sorry, I didn't understand your comment the last time about making changes with an eye on deprecation.

I will make the change but I don't understand the motivation here. Why do you consider removing the ResponseHandlerContext a breaking change? The class has simply been refactored (or its functionality decomposed into other classes) while maintaining the same business logic as before.

As part of Tinkerpop development, do we not delete anything without first marking it as deprecated, even if it is an internal class/function?

[spmallette]

We try not to create compile time problems during patch releases to make it as easy as possible for both providers and users to upgrade. I understand your use of "internal" there in that it's sort of a in-the-weeds sort of class but ResponseHandlerContext more strictly speaking it's not really an internal class in the sense that it was scoped public and then also part of methods that were public and therefore possibly utilized in some fashion (likely by providers and not so much end users). Obviously, if the class were scoped in a fashion that didn't allow folks to use it directly then there wouldn't be a problem with just removing it.

That's not to say that we don't make exceptions or never have breaking compile time changes (pretty rare I think which I like to take some pride in), but personally I prefer them more as a last resort as I imagine that few providers want to fuss very much with their implementations on older TinkerPop release lines (like tp33). Just trying to keep the wheels greased for upgrade as much as possible.

Is this a case for allowing the breaking change through? It's not a clear "yes" in my mind, since I think I see a pretty simple deprecation path. Of course, the fix for a provider who upgraded would be fast/easy plus they get a good bug fix for making the change, so if we let it go as-is, it wouldn't be the end of the world - some upgrade docs would probably be required for providers so that they were aware of the change and it would be all good.

So, sorry to make you shift one more time, but if you can take the deprecation approach without breaking compile time on upgrade, then let's do that. If you're sick of working on this ticket, I can finish it up for you. Just let me know.

For the future, prefer deprecation with no breaks in compilation/behavior on older branches. If there is no such path then we probably should discuss the approach to see if the community can live with the breaking change.

[spmallette]

Merged this to a new branch shown in PR #1168 - assuming that's ok, we will merge from there and close this one out.

[divijvaidya]

Sure thing. Thank you for taking it up and apologies for leaving it open for such a long time. Your suggestions regarding deprecation makes sense now, will take care in future code.