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.

Sign up for GitHub

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

Jump to bottom

Refactor ZonedDateTime.now in millis resolution #38577

Merged
merged 3 commits into from
Feb 8, 2019
Merged

Refactor ZonedDateTime.now in millis resolution #38577

merged 3 commits into from
Feb 8, 2019

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Feb 7, 2019 •
edited
Loading

In different places accross the code base the ZonedDateTime in
milliseconds resolution has to be used for testing. Refactoring this to
a single place to avoid code duplication

closes #38511
closes #38581

@pgomulka pgomulka added the WIP label Feb 7, 2019
@pgomulka pgomulka self-assigned this Feb 7, 2019
@pgomulka
Refactor ZonedDateTime.now in millis resolution
048422f 
In different places accross the code base the ZonedDateTime in
milliseconds resolution has to be used for testing. Refactoring this to
a single place to avoid code duplication
@pgomulka
fix index Action tests
a5bc498
@pgomulka pgomulka force-pushed the feature/milli_resolution_now branch from 88f2141 to a5bc498 Compare February 8, 2019 09:15
@pgomulka pgomulka added :Core/Infra/Core Core issues without another label :Analytics/SQL SQL querying :ml Machine learning :Data Management/Watcher labels Feb 8, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@pgomulka pgomulka removed the WIP label Feb 8, 2019
@pgomulka pgomulka requested a review from astefan February 8, 2019 13:01
astefan
astefan approved these changes Feb 8, 2019
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left just one cosmetics related comment.

...lugin/core/src/test/java/org/elasticsearch/protocol/xpack/watcher/GetWatchResponseTests.java
@@ -126,15 +124,15 @@ private static BytesReference simpleWatch() {

private static WatchStatus randomWatchStatus() {
long version = randomLongBetween(-1, Long.MAX_VALUE);
WatchStatus.State state = new WatchStatus.State(randomBoolean(), nowWithMillisResolution());
WatchStatus.State state = new WatchStatus.State(randomBoolean(), DateUtils.nowWithMillisResolution());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would import static this method to have a cleaner/nicer code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, I personally prefer to use a class name when context is not obvious. For the same reason I would use ZonedDateTime.now instead of now()

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java
@@ -29,7 +27,7 @@
public class ScheduledEventTests extends AbstractSerializingTestCase<ScheduledEvent> {

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

Choose a reason for hiding this comment

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

Same here for import static.

@pgomulka pgomulka merged commit c921a87 into elastic:master Feb 8, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 8, 2019
@jasontedor
Merge branch 'master' into docker-from-artifacts
4730a64 
* master:
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  Mute failing WatchStatusIntegrationTests (elastic#38621)
  Mute failing  ApiKeyIntegTests (elastic#38614)
  [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509)
  Add 7.1 and 8.0 version constants to master (elastic#38514)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 8, 2019
@jasontedor
Merge branch 'master' into llrc-java-7
b6612e9 
* master:
  Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636)
  add geotile_grid ref to asciidoc (elastic#38632)
  Enable Dockerfile from artifacts.elastic.co (elastic#38552)
  Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634)
  Account for a possible rolled over file while reading the audit log file (elastic#34909)
  Mute failure in InternalEngineTests (elastic#38622)
  Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518)
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  Mute failing WatchStatusIntegrationTests (elastic#38621)
  Mute failing  ApiKeyIntegTests (elastic#38614)
  [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509)
  Add 7.1 and 8.0 version constants to master (elastic#38514)
jasontedor added a commit to liketic/elasticsearch that referenced this pull request Feb 10, 2019
@jasontedor
Merge branch 'master' into pr/28895
e3508d1 
* master: (1159 commits)
  Fix timezone fallback in ingest processor (elastic#38407)
  Avoid polluting download stats on builds (elastic#38660)
  SQL: Prevent grouping over grouping functions (elastic#38649)
  SQL: Relax StackOverflow circuit breaker for constants (elastic#38572)
  [DOCS] Fixes broken migration links (elastic#38655)
  Drop support for the low-level REST client on JDK 7 (elastic#38540)
  [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641)
  fix dissect doc "ip" --> "clientip" (elastic#38545)
  Concurrent file chunk fetching for CCR restore (elastic#38495)
  make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473)
  SQL: Replace joda with java time (elastic#38437)
  Add fuzziness example (elastic#37194) (elastic#38648)
  Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636)
  add geotile_grid ref to asciidoc (elastic#38632)
  Enable Dockerfile from artifacts.elastic.co (elastic#38552)
  Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634)
  Account for a possible rolled over file while reading the audit log file (elastic#34909)
  Mute failure in InternalEngineTests (elastic#38622)
  Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518)
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 11, 2019
@jasontedor
Merge branch 'master' into retention-lease-ccr
affec57 
* master: (27 commits)
  Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636)
  add geotile_grid ref to asciidoc (elastic#38632)
  Enable Dockerfile from artifacts.elastic.co (elastic#38552)
  Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634)
  Account for a possible rolled over file while reading the audit log file (elastic#34909)
  Mute failure in InternalEngineTests (elastic#38622)
  Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518)
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  Mute failing WatchStatusIntegrationTests (elastic#38621)
  Mute failing  ApiKeyIntegTests (elastic#38614)
  [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509)
  Add 7.1 and 8.0 version constants to master (elastic#38514)
  ML: update set_upgrade_mode, add logging (elastic#38372)
  bad formatted JSON object (elastic#38515) (elastic#38525)
  Fix HistoryIntegrationTests timestamp comparsion (elastic#38505)
  SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440)
  Fix the clock resolution to millis in ScheduledEventTests (elastic#38506)
  Enable BWC after backport recovering leases (elastic#38485)
  Collapse retention lease integration tests (elastic#38483)
  TransportVerifyShardBeforeCloseAction should force a flush (elastic#38401)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefan astefan astefan approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying :Core/Infra/Core Core issues without another label :Data Management/Watcher :ml Machine learning
Projects
None yet
Milestone
No milestone
3 participants
@pgomulka @elasticmachine @astefan