You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/08/08 17:46:55 UTC
[GitHub] [tinkerpop] divijvaidya opened a new pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
divijvaidya opened a new pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309
https://issues.apache.org/jira/browse/TINKERPOP-2369
With this change, the following behaviour are modified:
1. If server closes a `Channel`, the client would detect it and replace the `Connection`.
Prior to this change, the client would do nothing on server initiated `Channel` close (applicable for cases when there were
no active requests on the channel). This would cause the load balancer to allocate requests to an already closed
Connection.
2. Do not throw a NPE when server sends a response for a request which is not registered with the client. Ignore it silently.
3. Ensure that replacement of a `Connection` is done only once. Prior to this change, for each failure of request using the
`Connection`, a new connection was being created.
**Testing**
Added tests which fail before the fixes and then succeed after the changes.
----------------------------------------------------------------
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
[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309#discussion_r467487980
##########
File path: gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
##########
@@ -191,14 +209,6 @@ Client getClient() {
return future;
}
- public void close() {
Review comment:
This is an unused function. Removed to simplify code.
----------------------------------------------------------------
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
[GitHub] [tinkerpop] divijvaidya merged pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
Posted by GitBox <gi...@apache.org>.
divijvaidya merged pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309
----------------------------------------------------------------
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
[GitHub] [tinkerpop] spmallette commented on pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309#issuecomment-678554629
VOTE +1
----------------------------------------------------------------
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
[GitHub] [tinkerpop] spmallette commented on pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
Posted by 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
[GitHub] [tinkerpop] divijvaidya commented on pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309#issuecomment-674327648
This is ready for review.
----------------------------------------------------------------
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
[GitHub] [tinkerpop] divijvaidya commented on pull request #1309: TINKERPOP-2369 Replace Connection on server initiated Channel close
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1309:
URL: https://github.com/apache/tinkerpop/pull/1309#issuecomment-677784939
1. Done.
2. I didn't find any. Do you have any specific places in mind?
4. I actually found quite a few bugs while debugging the weird issue with integ tests only failing when run together. Will fix them gradually in different PRs. The tricky part is to write reproducers for these edge cases which are triggered only during race conditions.
6. No. There weren't any server tests to test this scenario. The way how those tests are set up do not allow things like closing a connection. That is why I added the concept of `SimpleWebSocketServer` and associated tests. With these we can configure the behaviour of the server wrt Netty connection as needed for client testing.
7. Yes, I was hoping to do that on merge.
----------------------------------------------------------------
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