Reactive sql client pool in thread local #10221
Conversation
|
|
||
| @Override | ||
| public void beforeEach(ExtensionContext extensionContext) throws Exception { | ||
| rootLogger.addHandler(inMemoryLogHandler); |
There was a problem hiding this comment.
I'm not too familiar with loggers and junit extensions, but isn't this going to leak in the testsuite?
Looks like it will keep adding new InMemoryLogHandler instances, and also it's not emptying each of them after usage. I suppose the rootLogger is a global static so this might get nasty?
There was a problem hiding this comment.
@geoand any opinion? I copied this from your QuarkusProdModeTest.
There was a problem hiding this comment.
In QuarkusProdModeTest this is done in beforeAll, is there any specific reason why it was added to beforeEach here?
There was a problem hiding this comment.
Having it in beforeAll might be slightly better, but would still leak no?
There was a problem hiding this comment.
Sorry I don't follow, what would leak?
There was a problem hiding this comment.
I'm still not understanding what you mean by "but because each test replaces it, it shouldn't be a problem" but it's ok, we can avoid blocking this PR just because I don't get it :)
With @FroMage having added the afterEach I'm happy.
There was a problem hiding this comment.
I mean that each new instance of QuarkusDevModeTest will provide a new handler thus replacing the old one.
But for sure the best solution is to get a hold of the initial handlers in beforeAll and then restore them in afterAll
There was a problem hiding this comment.
it's adding handlers, not replacing existing ones.
There was a problem hiding this comment.
I'll take care of this in a separate PR
| @Override | ||
| public String apply(String s) { | ||
| return s.replace("quarkus.datasource.reactive.thread-local=true", | ||
| "quarkus.datasource.reactive.thread-local = true"); |
There was a problem hiding this comment.
what is the goal of this replacement? Might need a comment?
There was a problem hiding this comment.
Just to trigger a hot reload.
There was a problem hiding this comment.
Please add a comment. And is the change to the entity not enought to trigger one?
There was a problem hiding this comment.
No, it does not reload the pools. The entity change is to verify that a reload happened. I've added a comment.
There was a problem hiding this comment.
Ok thanks. Off topic, but I wonder if we should improve on that: the configuration isn't really different so there shouldn't be any need to reload :)
...pg-client/runtime/src/main/java/io/quarkus/reactive/pg/client/runtime/ThreadLocalPgPool.java
Outdated
Show resolved
Hide resolved
...ent/runtime/src/main/java/io/quarkus/reactive/mysql/client/runtime/ThreadLocalMySQLPool.java
Outdated
Show resolved
Hide resolved
...-client/runtime/src/main/java/io/quarkus/reactive/db2/client/runtime/ThreadLocalDB2Pool.java
Outdated
Show resolved
Hide resolved
...datasource/runtime/src/main/java/io/quarkus/reactive/datasource/runtime/ThreadLocalPool.java
Outdated
Show resolved
Hide resolved
...datasource/runtime/src/main/java/io/quarkus/reactive/datasource/runtime/ThreadLocalPool.java
Outdated
Show resolved
Hide resolved
...datasource/runtime/src/main/java/io/quarkus/reactive/datasource/runtime/ThreadLocalPool.java
Outdated
Show resolved
Hide resolved
| quarkus.datasource.username=hibernate_orm_test | ||
| quarkus.datasource.password=hibernate_orm_test | ||
| quarkus.datasource.reactive.url=${reactive-postgres.url} | ||
| quarkus.datasource.reactive.thread-local=true |
There was a problem hiding this comment.
The configuration property isn't suggesting that it relates to an enhanced connection pool.
Maybe threadlocal-pool ? Or pool-strategy= [enum: "threadlocal" | "simple" ] ?
There was a problem hiding this comment.
Fair enough, but max-size also relates to the pool without saying it, so…
There was a problem hiding this comment.
@gsmet how would you name it? You recently refactored all datasource configuration right?
There was a problem hiding this comment.
I've un-blocked the PR. Let's go ahead with the feature as we need it asap for benchmarking, but I'd still like to revisit the configuration in a follow up.
FroMage
force-pushed
the
pgpool-thread-local
branch
from
307b35a to
f66f865
Compare
FroMage
force-pushed
the
pgpool-thread-local
branch
from
f66f865 to
d2433d8
Compare
|
@FroMage, @Sanne I think we need to revert because CI failed the new test (and I can also reproduce the failure locally). Also the failure has nothing to do with the logs, the Here is the output: |
| private static final Logger log = Logger.getLogger(ThreadLocalPool.class); | ||
|
|
||
| private final AtomicReference<ThreadLocal<PoolType>> pool = new AtomicReference<>(new ThreadLocal<>()); | ||
| private static final List<Pool> threadLocalPools = new ArrayList<>(); |
There was a problem hiding this comment.
This could be a copy on write list and relieve from synchronization, given the list will not change often.
There was a problem hiding this comment.
BTW github UI seems confusing: we're now commenting on FroMage 's original PR but looking at the version of code I had modified further in a follow-up PR.. weird.
There was a problem hiding this comment.
ah nevermind, for some reason it looked like my follow up PR. Please check the new version, now in master ;)
| private PoolType pool() { | ||
| ThreadLocal<PoolType> poolThreadLocal = pool.get(); | ||
| PoolType ret = poolThreadLocal.get(); | ||
| if (ret == null) { |
There was a problem hiding this comment.
Since you have an AtomicReference, you could use compare and swap and avoid adding a superfluous ThreadLocalPool to the list if two threads execute this part concurrently.
There was a problem hiding this comment.
+1 I've done something similar in the follow-up PR, I was more concerned about making sure we'd not leak than getting most performance out of it: having chatted with @johnaohara we believe there's actually room for a better design (similar to what Agroal and Hikari have) and we should do that, but then contribute it into the pgclient project.
There was a problem hiding this comment.
Ok, looking forward to it
|
@FroMage could you please describe why do we need to use |
|
@robotmrv it's quite simple: the pool in vert.x v.3 (which is what we're using here currently) is not designed to work with multiple thread, but is designed to be used by a single thread. So, each thread should have its own. This will likely change when we switch to vert.x v4 |
|
BTW, configuration property |
I'm looking at the Are you saying that this change is only to "scale" But if so - it creates |
The "pool" is not used only by its own internals (which is indeed also expecting a single Thread), but within Quarkus there's multiple threads who are accessing it the pool.
Yes, this concern has been raised before, and the Vert.x team is working on a better pool implementation for Vert.x 4. |
sorry but I do not understand why it is not thread safe to use Query<RowSet<Row>> query(String sql);
PreparedQuery<RowSet<Row>> preparedQuery(String sql);
void begin(Handler<AsyncResult<Transaction>> handler);
void getConnection(Handler<AsyncResult<SqlConnection>> handler);
void close()By using I mean invoke above methods from any Thread, and then process callbacks without switching Thread. public void getConnection(Handler<AsyncResult<SqlConnection>> handler) {
Context current = Vertx.currentContext();
if (current == context) {
pool.acquire(new ConnectionWaiter(handler));
} else {
context.runOnContext(v -> getConnection(handler));
}
}so every command is running on a single Thread and Pool / Connection state is mutated by a single Thread. @Sanne
Or are you talking about hibernate reactive case that uses Pool internals and it is not thread safe? Sorry for asking so many questions This is how I see ThreadLocal solution |
|
@Sanne |
5 participants
Added the
quarkus.datasource.reactive.thread-localexperimental configuration, which stores the pool into a thread-local.