[GitHub] [tinkerpop] spmallette commented on pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close

[GitBox <gi...@apache.org>]

spmallette commented on pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309#issuecomment-677678867


   Thanks for this - a few minor comments/nits:
   
   1. Could you please rename tests that start with "Test*" to our more standard "should*"
   2. Have a look at where you might add more `final` declarations to match the code style.
   3. I like the idea of integration tests with the `SimpleWebSocketServer` - very smart
   4. I'm surprised you found as many places as you did where `Cluster.close()` wasn't called.
   5. I don't see where the semantics of any of the `GremlinDriverIntegrateTest` tests changed so it seems you accomplished this fix without breaking behavioral changes in the driver...that's nice.
   6. Maybe I missed it but was there a test in Gremlin Server to validate any of this change - perhaps it was already better tested by way of your `SimpleWebSocketServer` tests?
   7. I assume you will polish up the commit history a bit on merge and squash things down to a few (or one) commits?


----------------------------------------------------------------
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:
users@infra.apache.org