Require executor to be passed to lock refreshing timelock #2466
Conversation
tpetracca
changed the title
| @@ -36,7 +36,8 @@ | |||
| config.atlasDbConfig(), | |||
| resource -> { }, | |||
| LockServiceImpl::create, | |||
| () -> config.atlasDbSupplier().getTimestampService()); | |||
| () -> config.atlasDbSupplier().getTimestampService(), | |||
| null); //TODO | |||
There was a problem hiding this comment.
@jeremyk-91 @fsamuel-bs can someone help me figure out what to do here?
There was a problem hiding this comment.
We will probably want to create a @Qualifier annotation for the refreshing scheduled executor and add that as a param to this method, and provide a default implementation
There was a problem hiding this comment.
The cheap solution is to hardcode a default...
Better solution is to have a qualifier annotation for the refreshing scheduled executor as @schlosna mentions.
| @@ -142,6 +143,8 @@ String derivedUserAgent() { | |||
| return userAgent().orElse(callingClass().map(UserAgents::fromClass).orElse(UserAgents.DEFAULT_USER_AGENT)); | |||
| } | |||
|
|
|||
| abstract ScheduledExecutorService executor(); | |||
There was a problem hiding this comment.
Should probably name this more specifically as scheduledExecutor or even lockRefreshingExecutor
There was a problem hiding this comment.
If feels pretty random to just have this one executor passed in when we have many others that are just created internally. What's the reason for this?
There was a problem hiding this comment.
I think there was a plan to shift towards a model where we pass executors around, so that internally at least we can control overall thread-pool sizes. Don't really have full context though!
| .setNameFormat(TransactionManagersTest.class.getSimpleName() + "-%d") | ||
| .setDaemon(true) | ||
| .build()); | ||
|
|
There was a problem hiding this comment.
Should make this a memorized supplier so we don’t create it on class load, and we would still need to shut it down if we create it here.
There was a problem hiding this comment.
(I guess this is a test class so it might not be as critical, but agree.)
| @@ -36,7 +36,8 @@ | |||
| config.atlasDbConfig(), | |||
| resource -> { }, | |||
| LockServiceImpl::create, | |||
| () -> config.atlasDbSupplier().getTimestampService()); | |||
| () -> config.atlasDbSupplier().getTimestampService(), | |||
| null); //TODO | |||
There was a problem hiding this comment.
We will probably want to create a @Qualifier annotation for the refreshing scheduled executor and add that as a param to this method, and provide a default implementation
|
Is this needed for testability? If so what about just passing in a LockRefresher? |
tpetracca
force-pushed
the
tomp/tms-builder
branch
from
6fb75bc to
37b81fc
Compare
|
@tpetracca ping! |
|
I don't have super strong feelings, though ideally we would allow injection of all necessary executors so that we can ensure proper lifecycle management and instrumentation of resources. |
Goals (and why): Make consumer of refreshing locks stuff responsible for executor
Implementation Description (bullets):
nulland TODOConcerns (what feedback would you like?): Moar breaking changes!
Where should we start reviewing?:
Priority (whenever / two weeks / yesterday):
This change is