Skip to content

Refactor ZonedDateTime.now in millis resolution#38577

Merged
pgomulka merged 3 commits intoelastic:masterfrom
pgomulka:feature/milli_resolution_now
Feb 8, 2019
Merged

Refactor ZonedDateTime.now in millis resolution#38577
pgomulka merged 3 commits intoelastic:masterfrom
pgomulka:feature/milli_resolution_now

Conversation

@pgomulka
Copy link
Copy Markdown
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 :Distributed/Watcher labels Feb 8, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 8, 2019

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 8, 2019

Pinging @elastic/ml-core

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 8, 2019

Pinging @elastic/es-search

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 8, 2019

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
Copy Markdown
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
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
Copy Markdown
Contributor

@astefan astefan Feb 8, 2019

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
Copy Markdown
Contributor Author

@pgomulka pgomulka Feb 8, 2019

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

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

@astefan astefan Feb 8, 2019

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 :Distributed/Watcher :ml Machine learning

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] IndexActionTests.testIndexActionExecuteSingleDoc Fails Refactor the nowInMillisResolution in a common place

3 participants

@pgomulka @elasticmachine @astefan