Don't perform async initialization on calling thread #5746
Conversation
Generate changelog in
|
pkoenig10
force-pushed
the
pkoenig10/async
branch
from
b2bd497 to
2c3645e
Compare
|
Maybe we don't want this since it will introduce some additional overhead in the typical case. Digging into metrics a bit more, the slow initialization issue seems to be isolated to a couple installations, so this may be more of an environment issue. |
|
I was OOTO from last week Friday. Will get to this at some point this week. |
There was a problem hiding this comment.
Thanks for the PR! I have some comments on one of the tests, but in general I agree that this model is preferable. I'd like a quick second check from @gmaretic who had worked on this feature at least in a reviewing capacity back when it was implemented, in case there are any clients that might actually have depended on this specific behaviour.
| } | ||
|
|
||
| private static final class AlwaysFailingInitializerAssert | ||
| extends AbstractObjectAssert<AlwaysFailingInitializerAssert, AlwaysFailingInitializer> { |
There was a problem hiding this comment.
❤️ sometimes I feel we should use this more often in Atlas...
atlasdb-config/src/test/java/com/palantir/atlasdb/memory/InMemoryAsyncAtlasDbFactoryTest.java
Outdated
Show resolved
Hide resolved
| TimestampService timestampService = factory.createManagedTimestampService(kvs, Optional.empty(), true); | ||
|
|
||
| assertThat(timestampService.isInitialized()).isFalse(); | ||
| assertThatThrownBy(timestampService::getFreshTimestamp).isInstanceOf(NotInitializedException.class); |
There was a problem hiding this comment.
oof, looks like this assertion failed on CircleCI :/ would it make sense to pass the executor for async init in?
There was a problem hiding this comment.
unfortunately the race condition here looks hard to avoid
There was a problem hiding this comment.
(I realise this kind of went out of the scope of the original change, if you'd prefer someone on the team to do this instead, just let me know)
There was a problem hiding this comment.
Yeah I noticed this when making this change. I was surprised the KVS test hasn't flaked before.
I'm happy to fix this. Like you said, the race condition here is hard to avoid. Do you have a suggestion? These tests don't seem that useful to me, I'd probably just remove them.
There was a problem hiding this comment.
Or we can remove just the not initialized assertions.
pkoenig10
force-pushed
the
pkoenig10/async
branch
from
5629dfa to
281ec20
Compare
|
Released 0.469.0 |
Goals (and why):
Enable faster server initialization when using
initializeAsyncIn large internal authentication service, we've noticed that some installations take up to 4 minutes for the server to be initialized. Logs indicate that
CassandraClientPoolImplinitialization is happening before the server is initialized and takes a majority of those 4 minutes.Implementation Description (bullets):
Before this PR, when
initializeAsyncis true, AtlasDB attempts to initialize components synchronously on the calling thread. If the initialization takes a long time (like it does forCassandraClientPoolImpl), then this will block other initialization work.After this PR, when
initializeAsyncis true, AtlasDB will schedule the initialization to run immediately on the executor thread. This way other initialization can continue while the AtlasDB initialization happens in the background.This seems to be more intuitive behavior for a flag named
initializeAsync.Testing (What was existing testing like? What have you done to improve it?):
I've updated
AsyncInitializerTestto verify this behavior. I've also refactored this test class to make it a bit more readable.