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

Switch to JDK date time library #1418

Merged
merged 4 commits into from Jan 5, 2021
Merged

Conversation

dsyer
Copy link
Contributor

@dsyer dsyer commented Dec 4, 2020

See kubernetes-client/gen#177 for
the corresponding change in the "gen" project.

Fixes #1411 (but N.B. this is a binary incompatible change).

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dsyer
To complete the pull request process, please assign brendandburns after the PR has been reviewed.
You can assign the PR to them by writing /assign @brendandburns in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 4, 2020
Copy link
Member

@yue9944882 yue9944882 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmmm, all the tests pass so there's no problem talking to an e2e kubernetes cluster in terms of [de]serialization.

if we're merging this, i think we should provide utilities for jodatime <-> offsetdatetime conversion so that users can bandage the incompatible changes. @brendandburns @dsyer wdyt?

@dsyer
Copy link
Contributor Author

dsyer commented Dec 4, 2020

What kind of utilities were you thinking of? It seems like we encourage users to generate their API objects from YAML? If so then the conversion should be automatic (just upgrade the libraries and re-generate your model objects). I suppose there might be some model objects that need to be manipulated in user code, and they might contain date time fields. The conversion of the code in this project was largely trivial, by the way (Java 8 date time library evolved from joda-time so that's not surprising), and I expect users would experience the same level of toil. It is possible to automate such tasks. Not sure if it's worth it.

@yue9944882
Copy link
Member

for clarification, i was thinking about something like the code snippet below. and the point is to help users do less work in the migration. e.g. users can be already using joda-time in the other non-kubernetes models in their legacy system and they don't have to switch the presence of joda-time everywhere. just think about the kids :)

public class DateTimeUtilities {
   public static toOffsetDateTime(JodaTime time) {...}
   public static toJodaTime(OffsetDateTime time) {...}
}

@brendandburns
Copy link
Contributor

Agree, this is a breaking change for users of this library and so I'd like to make the transition easier via some utilities.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2020
@yue9944882
Copy link
Member

@dsyer how do you think about reviving the thread?

@dsyer
Copy link
Contributor Author

dsyer commented Dec 31, 2020

If you think it would work as a new major version I can rebase onto master I guess. I’m on PTO till Monday.

@yue9944882
Copy link
Member

If you think it would work as a new major version I can rebase onto master I guess. I’m on PTO till Monday.

thanks, we're hitting an issue that joda-time doesn't support micro/nano sec timestamp, replacing it in the next release sounds good to me. cc @brendandburns

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2021
@yue9944882
Copy link
Member

@dsyer btw can you re-generate together w/ 1.20 kubernetes? (by bumping source branch name to release-1.20)

java/settings

Line 18 in a8e52f7

export KUBERNETES_BRANCH="release-1.19"

@dsyer
Copy link
Contributor Author

dsyer commented Jan 4, 2021

I rebased. Also added a further commit with the k8s version change (so you can leave that off if you want).

@dsyer
Copy link
Contributor Author

dsyer commented Jan 4, 2021

FWIW the LeaderElectorTest works for me locally. The CSRTest does not but I don't really know why and that one seems to work in CI anyway. Seems like the tests might be flickering?

@brendandburns
Copy link
Contributor

I bounced the tests, we'll see if there was a flake.

@dsyer
Copy link
Contributor Author

dsyer commented Jan 4, 2021

That didn't help then. The CI failures are different to my local failures still. 🤷‍♀️

Not sure how to debug that.

@brendandburns
Copy link
Contributor

Chances are the test is racy. Usually the best way to repro that I have found is to run the test on a really, really low powered cloud VM (e.g. Azure B-series or AWS *.micro series)

@brendandburns
Copy link
Contributor

I actually see LeaderElectorTest failing for me locally (timeout) at HEAD, I will attempt to debug on my local machine.

@brendandburns
Copy link
Contributor

@dsyer have you rebased this PR to HEAD?

@dsyer
Copy link
Contributor Author

dsyer commented Jan 5, 2021

I rebased yesterday. HEAD has moved of course, but that shouldn't affect the tests right?

Signed-off-by: Dave Syer <dsyer@vmware.com>
Signed-off-by: Dave Syer <dsyer@vmware.com>
Signed-off-by: Dave Syer <dsyer@vmware.com>
Signed-off-by: Dave Syer <dsyer@vmware.com>
Copy link
Member

@yue9944882 yue9944882 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyer thanks! the pull looks good now, the remaining e2e failure is caused by unwounded hacks for joda-time formatter which i suppose is irrelavant to this pull. i will deal w/ the e2e failure in a follow up.

https://github.com/kubernetes-client/java/blob/master/e2e/src/test/java/io/kubernetes/client/e2e/extended/leaderelection/LeaderElectorTest.java#L88-L101

@yue9944882 yue9944882 merged commit 6267ff3 into kubernetes-client:master Jan 5, 2021
@dsyer
Copy link
Contributor Author

dsyer commented Jan 5, 2021

Those changes in LeaderElectorTest shouldn't be necessary with JDK time. OffsetDateTime.parse() works with "2020-01-04T04:34:56Z" or "2020-01-04T04:34:56.001Z" or "2020-01-04T04:34:56.001002Z".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use JDK for date time not Joda
4 participants