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

RecoveryMonitor#lastSeenAccessTime should be volatile #36781

Merged
merged 4 commits into from
Dec 21, 2018
Merged

RecoveryMonitor#lastSeenAccessTime should be volatile #36781

merged 4 commits into from
Dec 21, 2018

Conversation

DaveCTurner
Copy link
Contributor

This local field is accessed on multiple threads and is nonvolatile so
theoretically could yield stale values. Not sure it does in practice.

@DaveCTurner
RecoveryMonitor#lastSeenAccessTime should be volatile
b72d01a 
This local field is accessed on multiple threads and is nonvolatile so
theoretically could yield stale values. Not sure it does in practice.
@DaveCTurner DaveCTurner added >bug :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v7.0.0 v6.6.0 labels Dec 18, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@jasontedor jasontedor added v6.7.0 and removed v6.6.0 labels Dec 19, 2018
@DaveCTurner DaveCTurner requested a review from ywelsch December 20, 2018 15:32
@DaveCTurner
Copy link
Contributor Author

run Gradle build tests 1
run Gradle build tests 2

@DaveCTurner
Copy link
Contributor Author

run the Gradle build tests 2

ywelsch
ywelsch approved these changes Dec 20, 2018
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM. I think not much harm is done if value is stale, but volatile is better.

@DaveCTurner DaveCTurner merged commit cfea2fd into elastic:master Dec 21, 2018
@DaveCTurner DaveCTurner deleted the 2018-12-18-RecoveryMonitor-missing-volatile branch December 21, 2018 11:10
DaveCTurner added a commit that referenced this pull request Dec 21, 2018
@DaveCTurner
RecoveryMonitor#lastSeenAccessTime should be volatile (#36781)
d622466 
This local field is accessed on multiple threads and is nonvolatile so
theoretically could yield stale values. Not sure it does in practice.
jasontedor added a commit to ywelsch/elasticsearch that referenced this pull request Dec 21, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into pr/36137
8de54b8 
* elastic/master: (539 commits)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  Enable IPv6 URIs in reindex from remote (elastic#36874)
  Watcher: Remove unused local variable in doExecute (elastic#36655)
  [DOCS] Synchs titles of X-Pack APIs
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 21, 2018
@jasontedor
Merge branch 'master' into package-ingest-geoip-as-a-module
30d92d1 
* master: (31 commits)
  Move ingest-geoip default databases out of config (elastic#36949)
  [ILM][DOCS] add extra scenario to policy update docs (elastic#36871)
  [Painless] Add String Casting Tests (elastic#36945)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywelsch ywelsch ywelsch approved these changes

Assignees
No one assigned
Labels
>bug :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v6.7.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DaveCTurner @elasticmachine @ywelsch @colings86 @jasontedor