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

Refactoring scheduled event to store instant instead of zoned tme zone #39380

Conversation

Mehrank64
Copy link
Contributor

@Mehrank64 Mehrank64 commented Feb 25, 2019

The ScheduledEvent class has never preserved the time
zone so it makes more sense for it to store the start and
end time using Instant rather than ZonedDateTime.

Closes #38620

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@benwtrent
Copy link
Member

Jenkins retest this please

@benwtrent
Copy link
Member

@Mehrank64 Thank you so much for opening the PR 🚀 . Here are some gradle tasks to see if all the preCommit linting passes and the basic unit tests.

./gradlew :x-pack:plugin:core:preCommit :x-pack:plugin:ml:preCommit :x-pack:plugin:core:test :x-pack:plugin:ml:test

It looks like some linting failures are causing the builds to fail.

@@ -27,7 +26,7 @@
public class ScheduledEventTests extends AbstractSerializingTestCase<ScheduledEvent> {

public static ScheduledEvent createScheduledEvent(String calendarId) {
ZonedDateTime start = DateUtils.nowWithMillisResolution();
Instant start = Instant.ofEpochSecond(Instant.now().toEpochMilli());
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a discrepancy here - it's interpreting a millisecond number as seconds, resulting in a time long in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was definitely a mistake. just solved!

@droberts195
Copy link
Contributor

Regarding the wildcard imports, which the style checker doesn't like, there is advice on how to stop IntelliJ switching to wildcards in https://github.com/elastic/elasticsearch/blob/master/CONTRIBUTING.md:

  • IntelliJ: Preferences/Settings->Editor->Code Style->Java->Imports. There are two configuration options: Class count to use import with '*' and Names count to use static import with '*'. Set their values to 99999 or some other absurdly high value.

Hopefully if you change that setting and then re-save the files from IntelliJ it will remove the wildcard imports.

@Mehrank64
Copy link
Contributor Author

@Mehrank64 Thank you so much for opening the PR 🚀 . Here are some gradle tasks to see if all the preCommit linting passes and the basic unit tests.

./gradlew :x-pack:plugin:core:preCommit :x-pack:plugin:ml:preCommit :x-pack:plugin:core:test :x-pack:plugin:ml:test

It looks like some linting failures are causing the builds to fail.

@Mehrank64 Mehrank64 closed this Feb 26, 2019
@Mehrank64
Copy link
Contributor Author

@benwtrent I did resolve the linting failures and the now all the tests should pass. I closed the PR by accident and can't find any way to re-open it. will raise another PR.

@droberts195 droberts195 reopened this Feb 26, 2019
@droberts195
Copy link
Contributor

Jenkins retest this please

@benwtrent
Copy link
Member

Jenkins retest this please

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution @Mehrank64 - much appreciated!

@droberts195 droberts195 merged commit fe7b215 into elastic:master Feb 27, 2019
droberts195 pushed a commit that referenced this pull request Feb 27, 2019
…me zone (#39380)

The ScheduledEvent class has never preserved the time
zone so it makes more sense for it to store the start and
end time using Instant rather than ZonedDateTime.

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

Successfully merging this pull request may close these issues.

[ML] ScheduledEvent takes and stores ZonedDateTime but then discards the timezone
5 participants