Skip to content
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

Use JDK for date time not Joda #1411

Closed
dsyer opened this issue Dec 3, 2020 · 5 comments · Fixed by #1418
Closed

Use JDK for date time not Joda #1411

dsyer opened this issue Dec 3, 2020 · 5 comments · Fixed by #1418

Comments

@dsyer
Copy link
Contributor

dsyer commented Dec 3, 2020

Joda is not a good choice for a modern library. I doubt if anyone who uses the Kubernetes SDK needs it. Unfortunately it's not easy to get rid of because some of the generated code uses it. You can change the options in the code generator (https://openapi-generator.tech/docs/generators/java/) so that's what we should do I believe, but that involves changes to the "gen" project in kubernetes-client to be co-ordinated with this project. I don't even know how to start that process, so someone will have to make a suggestion.

@brendandburns
Copy link
Contributor

You can definitely file PRs on the gen repo and then they'll get picked up the next time we regenerate the code. We do that all the time.

@dsyer
Copy link
Contributor Author

dsyer commented Dec 3, 2020

It's not that simple unfortunately. The change required in "gen" is trivial, but it would break the build here because of the JSON utility class (mostly). I don't understand that code well enough to see how you could fix it. Is it originally auto-generated but now somehow manually maintained?

The (trivial) required change in "gen": kubernetes-client/gen#177

Consequent (big) change in this project: #1418

@dsyer
Copy link
Contributor Author

dsyer commented Dec 4, 2020

Also, this is a binary incompatible change, not only for the generated API objects, but also e.g. methods like ProcessorListener.shouldResync(). So I think we should do it, but it would have to be version 11 and the change would have to be documented and socialized properly. Would that work?

@yue9944882
Copy link
Member

am not sure why we picked joda time in the past, but given that it's been there for a period of time, migrating to the standard date library will be incompatible and costy. e.g. switching the type for all the utility classes @dsyer can you elaborate why we need to this specifically? i'm leaning on the remaining the current state if it's just for looking good aesthetically.

besides, we uses a custom gson adapter for joda time types so that the serialized timestamp on the kubernetes resources can be compatible with the kubernetes builtin serialization. using jodatime types will tighten the scope of the gson adapter to the kubernetes resources, otherwise the adapter will be working for an the non-kubernetes model classes using standard datetime class.

@dsyer
Copy link
Contributor Author

dsyer commented Dec 4, 2020

From the Joda README:

Joda-time is no longer in active development

and

From Java SE 8 onwards, users are asked to migrate to java.time (JSR-310) - a core part of the JDK which replaces this project.

Since Kubernetes client only supports Java 8 and above I think it's harmful to expose users to a legacy library that has been superseded by the core runtime. I struggle to conceive how joda-time was ever chosen as a date time library for this SDK.

For sure it is a big change (as I already said, and demonstrated), but I really think it's worth it.

we uses a custom gson adapter for joda time types so that the serialized timestamp on the kubernetes resources can be compatible with the kubernetes builtin serialization

I'm pretty sure that the JDK date time utilities are already compatible - the tests are all green anyway. Are there some missing tests that assert the Gson outputs? I'm not sure what you mean by the last sentence ("tighten the scope" etc.).

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 a pull request may close this issue.

3 participants