Skip to content

Added factory class for InstantSource bean creation #11774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: 4.9.x
Choose a base branch
from

Conversation

bfg
Copy link
Contributor

@bfg bfg commented Apr 23, 2025

For motivation and rationale see #11655

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2025

CLA assistant check
All committers have signed the CLA.

@bfg bfg force-pushed the time_source_bean branch from 173634f to 3bd26c5 Compare April 23, 2025 23:58
@bfg bfg changed the title Added new interface: InstantSource Added factory class for InstantSource bean creation Apr 23, 2025
@bfg
Copy link
Contributor Author

bfg commented Apr 24, 2025

Hey @yawkat, I've created a simple draft for this; I have few questions

  • did I place factory class in a correct module and package?
  • maybe we should consider creating full Clock type in the future so that client code can query timezone as well?

@bfg bfg force-pushed the time_source_bean branch 2 times, most recently from 12ab9da to 8513c01 Compare April 24, 2025 07:00
@yawkat
Copy link
Member

yawkat commented May 7, 2025

The location seems reasonable to me. @graemerocher WDYT

I don't think we should expose a full Clock. The server time zone is not something that microservices should touch anyway.

@bfg
Copy link
Contributor Author

bfg commented May 7, 2025

The location seems reasonable to me. @graemerocher WDYT

I don't think we should expose a full Clock. The server time zone is not something that microservices should touch anyway.

just FYI: I've searched the code and java.time.ZoneId is imported in the following classes:

./http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/element/DateTimeElement.java
./context/src/main/java/io/micronaut/scheduling/annotation/Scheduled.java
./context/src/main/java/io/micronaut/scheduling/NextFireTime.java
./context/src/main/java/io/micronaut/scheduling/ScheduledExecutorTaskScheduler.java
./context/src/main/java/io/micronaut/runtime/converters/time/TimeConverterRegistrar.java
./core/src/main/java/io/micronaut/core/reflect/ClassUtils.java
./http/src/main/java/io/micronaut/http/MutableHttpHeaders.java
./http/src/main/java/io/micronaut/http/HttpHeaders.java
./http-netty/src/main/java/io/micronaut/http/netty/NettyHttpHeaders.java

... I guess that scheduling package might need zone, because it's using system default. Dunno, we might be able to work with system default.

In addition it's being used in micronaut-data as well; system default zone is being used by default.

./data-r2dbc/src/main/java/io/micronaut/data/r2dbc/mapper/ColumnIndexR2dbcResultReader.java:import java.time.ZoneId;
./data-r2dbc/src/main/java/io/micronaut/data/r2dbc/mapper/ColumnNameR2dbcResultReader.java:import java.time.ZoneId;
./data-runtime/src/main/java/io/micronaut/data/runtime/convert/DataConversionServiceFactory.java:import java.time.ZoneId;
./data-document-model/src/main/java/io/micronaut/data/document/model/query/builder/MongoQueryBuilder.java:import java.time.ZoneId;
./data-document-model/src/main/java/io/micronaut/data/document/model/query/builder/MongoQueryBuilder2.java:import java.time.ZoneId;

@bfg bfg force-pushed the time_source_bean branch from 8513c01 to 329a505 Compare May 8, 2025 11:43
@bfg bfg force-pushed the time_source_bean branch from 329a505 to 787f09f Compare May 8, 2025 11:53
@mikehearn
Copy link
Contributor

The PR looks good and I've implemented something similar in my own project that uses Micronaut. Could you maybe take it out of draft (and I guess now, rebase to 4.10.x as it's new API?). I think when a PR is marked as in draft, the maintainers don't look at it.

@yawkat
Copy link
Member

yawkat commented Jun 23, 2025

There's also a new Ticker API in netty that it would be cool to integrate with: https://github.com/netty/netty/pull/15026/files#diff-df3131658fad36f38d0be80c7870224d302810296ef4a42b79ffe02113443a8a

But since that is nanoTime-based, I'm not sure it's feasible.

@bfg
Copy link
Contributor Author

bfg commented Jun 23, 2025

@mikehearn I was/still am waiting for feedback 🤷 In my project I've included Clock bean.
@yawkat I guess nanoTime is actually not needed in the framework

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants