Core: shared clock #1416
Core: shared clock #1416
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1416 +/- ##
=============================================
- Coverage 67.61% 10.08% -57.53%
- Complexity 0 452 +452
=============================================
Files 719 719
Lines 15936 15957 +21
Branches 1043 1044 +1
=============================================
- Hits 10775 1610 -9165
- Misses 4687 14260 +9573
+ Partials 474 87 -387
Continue to review full report at Codecov.
|
algattik
requested review from
paullatzelsperger,
bscholtes1A and
jimmarino
as code owners
| @@ -47,28 +48,14 @@ public DefaultServiceExtensionContext(TypeManager typeManager, Monitor monitor, | |||
| registerService(TypeManager.class, typeManager); | |||
| registerService(Monitor.class, monitor); | |||
| registerService(Telemetry.class, telemetry); | |||
| registerService(Clock.class, Clock.systemUTC()); | |||
There was a problem hiding this comment.
Unless there is a good reason to the contrary we should provide the clock with a @Provider(isDefault=true)-annotated factory method in the DefaultServicesExtension.
There was a problem hiding this comment.
The reasons for this are:
- to provide a utility method
ServiceExtensionContext#getClockso that extensions don't need to inject a clock service. If we move the default clock toDefaultServicesExtension, we can't have such a utility method, since we need the dependency resolver to start extensions in the right order. - that the clock is a common service just like
TypeManager,MonitorandTelemetry, and there's no obvious reason to manage it differently
There was a problem hiding this comment.
Great stuff @algattik. I'd like to have a Decision Record since it is important to have this approach documented and everyone aware across the codebase.
Given the conflict issue, that could be done in a subsequent PR, even though it would have been better to commit the DR first.
|
@juliapampus no, this is a pure "internal utils" change and does not affect any contracts or behaviours |
Great point, I've updated the parent issue to add the need for a DR, and removed the "Closes" comment from this PR. I'll do a DR next. |
algattik
force-pushed
the
feature/1364-clock
branch
from
6c93f72 to
e218c47
Compare
* Feature/1364/284 clock (#286) * Checkstyle * Update CHANGELOG.md * Moved default clock to DefaultServicesExtension * Update DefaultServiceExtensionContext.java * Update SqlContractNegotiationStoreExtensionTest.java * Update AbstractMultipartDispatcherIntegrationTest.java * Revert "Update AbstractMultipartDispatcherIntegrationTest.java" This reverts commit 1ebb12f. * Revert "Update SqlContractNegotiationStoreExtensionTest.java" This reverts commit af4613b. * Revert "Update DefaultServiceExtensionContext.java" This reverts commit 3798770. * Revert "Moved default clock to DefaultServicesExtension" This reverts commit 7a9fa08. * Update ContractNegotiationIntegrationTest.java * Update ContractNegotiationIntegrationTest.java
What this PR changes/adds
Use a mockable
Clockto get the time.Removes calls to
Instant.now(),new Date(),System.currentTimeMillis()from production code.Why it does that
Further notes
DefaultServiceExtensionContextto the interfaceDataPlaneInstancecontract to return null instead of 0 for emptylastActivetime.nbf: in discussion with BenrollbackStatemethod in ContractNegotiation and TransferProcess, as it's unused and violates state machine contract.ContractServiceExtensionLinked Issue(s)
Contributes to #1364 (a decision record will also be provided)
Checklist
no-changelog)