Fixed issue where disconnect was not reconnecting and added tests #42
Conversation
|
With a couple more pull requests coming in by @jaceksokol, can this PR be revisited? |
|
Bump @jaceksokol (and @jaroslawk might be interested too). |
There was a problem hiding this comment.
I am impressed by your testing code which verify this validator - I did not know than one can kill the DB like that.
From what I have seen in my experience hanging query problem is sourced in the netty integration - to be more precise no connection/read timeouts.
| } | ||
| }); | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
it looks like you got two validation in this file - consider separating them, they do not need to have fields - just function with params.
Also I wander if you need the second one - when you are able to query does it mean you are able to connect?
There was a problem hiding this comment.
In my case, I didn't have a validation query, only a socket validation. They are separate, but I figured placing all forms of validation in ConnectionValidator was reasonable since it's the validation result that determines whether it needs to be refreshed regardless of the method you choose to validate. I was avoiding an over-abstraction here but we could easily abstract out many ways to validate a connection.
There was a problem hiding this comment.
It just looks like a two separate way of doing validation. Possibly one could have an option to use each one of them etc. This is just general observation about it.
There was a problem hiding this comment.
You should firstly check if there is socket connection then if you can query.
There was a problem hiding this comment.
Basically this socket check doesn't make much sense as if it is closed then the check query will fail.
There was a problem hiding this comment.
@jaceksokol - I explained this above with "I didn't have a validation query, only a socket validation." I don't want query validation. The socket dies underneath and I don't need a full query to the DB for the client side to detect socket death. People should not have to run queries against the DB to see if the client-side of the socket has died.
I just want the client-side socket death check (without requiring a validation query) and I figured the connection validator is the same place. I put it after because I don't want to change anything for how people rely on things today, just need to tack it on.
| Observable<Connection> ret = Observable.just(connection); | ||
| if (validationQuery != null) { | ||
| ret = ret.flatMap(conn -> connection.queryRows(validationQuery) | ||
| .lift(subscriber -> new Subscriber<Row>() { |
There was a problem hiding this comment.
how about instead of this code not do:
return connection.queryRows(validationQuery)
.map( __-> connection);
?
it will return connection, ignore the query result and in case of any errors propagate them forward.
There was a problem hiding this comment.
My goal here, in adding my socket validation, was to leave the existing logic as untouched as possible to avoid breaking any downstream code expectations. Therefore I simply refactored out what was there and added my check. Your comment applies to the original code as well and, to keep w/ minimal refactoring principles, maybe should be addressed there (i.e. open a PR addressing those concerns w/ the original source).
There was a problem hiding this comment.
I agree with your assumptions. It looks tempting to just remove all this code here :)
I was having an issue where I was not supplying a validation query, but the connection was actually dying (e.g. postgres restart) and it was just hanging with internal "Channel is closed" errors. I went ahead and added an opt-in config to make sure the connection is connected so as not to break BC. I also added tests for it and added tests for the validation query too.