-
Notifications
You must be signed in to change notification settings - Fork 15
TKVSM initialization handles when runtimeConfig is provided as a Supplier #7134
Conversation
Generate changelog in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot! I think we already do this resolution of the two suppliers earlier in the method though (back on L375), and so we should just use the results.
Basically, we shouldn't be calling runtimeConfig()
or runtimeConfigSupplier()
directly outside of where we resolve it to a more canonical thing.
(Also, as a drive by: it would be good to, while you're here, also clean up the other misuse of runtimeConfig(), which is in setUpMetricsAndGetMetricsManager().)
.get() | ||
.transactionKeyValueService() | ||
.get()), | ||
.orElseThrow(() -> new SafeIllegalArgumentException( | ||
"TransactionKeyValueServiceRuntimeConfig must be provided"))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted - this does look like a mistake we made in #6984 😅
That said, I'm not sure this is the right way to fix it - we already wrap the runtime config in a Refreshable on lines 372-373. Notice that other code around this uses runtime
rather than runtimeConfig()
or runtimeConfigSupplier()
. So I think here this should just be runtime.map(config -> config.tkvs().orElseThrow(...))
.
@@ -449,6 +449,9 @@ private TransactionManager serializableInternal(@Output List<AutoCloseable> clos | |||
() -> { | |||
TransactionKeyValueServiceManager tkvsm; | |||
if (config().transactionKeyValueService().isPresent()) { | |||
Refreshable<Optional<AtlasDbRuntimeConfig>> runtimeConfig = runtimeConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hoist this up somewhere?
I've refactored this to use the existing runtimeConfig <-> runtimeConfigSupplier resolution logic |
63dc3eb
to
5bb63d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yep, looks good!
General
Before this PR:
TransactionKeyValueServiceManager initialization assumes that the Atlas runtime config is provided as a
Refreshable
.We also support users providing the config as a
Supplier
under theruntimeConfigSupplier
field.After this PR:
This PR properly handles TKVSM initialization when the runtime config is passed using
runtimeConfigSupplier
. It is already asserted elsewhere that runtimeConfig XOR runtimeConfigSupplier is present.==COMMIT_MSG==
TKVSM initialization handles when runtimeConfig is provided as a Supplier
==COMMIT_MSG==
Please tag any other people who should be aware of this PR:
@jeremyk-91
@jkozlowski