Java 11 #64
Java 11 #64
Conversation
|
|
That's a lot of work, good job. You basically did what I did with https://github.com/cretz/pgnio after finding bugs here and realizing it was basically abandoned. |
|
This work is intended to fill the gap in async database connectivity for my other project - Septima Your project is cool. Especially I like support of COPY protocol. |
| return pooledConnection.completeScript(validationQuery) | ||
| .handle((rss, th) -> { | ||
| if (th != null) { | ||
| return ((PooledPgConnection) pooledConnection).delegate.close() |
There was a problem hiding this comment.
Does this leave the pool size at the wrong value?
There was a problem hiding this comment.
Basically, no. The exceptionally completed Future is returned here and than it composed later. It should lead to .exceptionally handler been invoked later. That handler has logic of decreasing of pool size if any exception occurs. But, i haven't checked this by my own eye yet. So, it should be checked. Fortunatelly, there is a test for bad validationQuery that can be examined with a debugger & eye.
There was a problem hiding this comment.
Indeed, exceptions are handled in a general manner here and pool size is decreased correctly. I've checked this by hand.
| @@ -0,0 +1,15 @@ | |||
| package com.pgasync; | |||
There was a problem hiding this comment.
The new package structure is proposed to get rid of clumsy name impl and to be able to give an implementation a name like a github.
If you have thoughts about more gracefull package structure, than i'll refactor it with pleasure.
|
|
||
| private final PgConnection delegate; | ||
| private PooledPgPreparedStatement evicted; | ||
| private final LinkedHashMap<String, PooledPgPreparedStatement> statements = new LinkedHashMap<>() { |
There was a problem hiding this comment.
LinkedHashMap is not thread safe but is mutated without any locking
There was a problem hiding this comment.
Check the JavaDoc on com.pgasync.Connection interface please. Connections are desined to be never thread safe. So, it is not an issue since statements map is a PooledPgConnection instance field.
There was a problem hiding this comment.
Ah, you're correct, I tripped on the deep nesting of classes.
| counter = 0; | ||
| prefix = "_" + prefix; | ||
| } | ||
| return prefix + ++counter; |
There was a problem hiding this comment.
Not thread-safe, counter should be an AtomicLong.
There was a problem hiding this comment.
Yes, currently it is a bug. But there is another option. According to Postgres protocol, prepared statements exist within a Postgres session only, and so wouldn't be better to refactor the only use case of NameSequence and make it instance non static? This will help us to avoid counter contention.
There was a problem hiding this comment.
Yeah, that sounds like a better choice.
|
@alaisi Do you consider to add more review comments or improvement proposals? |
|
Reviewed and merged, thanks for the effort! |
No description provided.