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

Fully encapsulate LocalCheckpointTracker inside of the engine #31213

Merged
merged 4 commits into from
Jun 8, 2018
Merged

Fully encapsulate LocalCheckpointTracker inside of the engine #31213

merged 4 commits into from
Jun 8, 2018

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Jun 8, 2018

This makes the Engine interface not expose the LocalCheckpointTracker, instead
exposing the pieces needed (like retrieving the local checkpoint) as individual
methods.

@dakrone
Fully encapsulate LocalCheckpointTracker inside of the engine
cc16abd 
This makes the Engine interface not expose the `LocalCheckpointTracker`, instead
exposing the pieces needed (like retrieving the local checkpoint) as individual
methods.
@dakrone dakrone added >non-issue v7.0.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v6.4.0 labels Jun 8, 2018
@dakrone dakrone requested a review from bleskes June 8, 2018 17:58
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dakrone dakrone mentioned this pull request Jun 8, 2018
Merged
bleskes
bleskes suggested changes Jun 8, 2018
View reviewed changes
Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

Thank you @dakrone ! I left one comment about generateSeqNo that I think is important. The rest is just a suggestion.

server/src/main/java/org/elasticsearch/index/engine/Engine.java Outdated
/**
* @return the maximum sequence number from the local checkpoint tracker
*/
public abstract long getMaxSeqNo();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can remove this? it's only used in one place where I think we can just use getSeqNoStats?

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly, I've done that, passing in -1 for the globalCheckpoint

server/src/main/java/org/elasticsearch/index/engine/Engine.java Outdated
/**
* @return generate a new sequence number from the local checkpoint tracker
*/
public abstract long generateSeqNo();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is super dangerous IMO. Maybe we can make getLocalCheckpointTracker package private and have a testing utility to get it? I think that's good enough for tests and we can keep it isolated from production code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I've removed this

@dakrone dakrone requested a review from bleskes June 8, 2018 20:15
bleskes
bleskes approved these changes Jun 8, 2018
View reviewed changes
Copy link
Contributor

@bleskes bleskes 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 @dakrone

@dakrone dakrone added review and removed review labels Jun 8, 2018
@dakrone dakrone merged commit bdb0fb2 into elastic:master Jun 8, 2018
dakrone added a commit that referenced this pull request Jun 9, 2018
@dakrone
Fully encapsulate LocalCheckpointTracker inside of the engine (#31213)
ea223c3 
* Fully encapsulate LocalCheckpointTracker inside of the engine

This makes the Engine interface not expose the `LocalCheckpointTracker`, instead
exposing the pieces needed (like retrieving the local checkpoint) as individual
methods.
dnhatn added a commit that referenced this pull request Jun 10, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
7ab6f92 
* 6.x:
  Move default location of dependencies report (#31228)
  Remove dependencies report task dependencies (#31227)
  Add recognition of MPL 2.0 (#31226)
  Fix unknown licenses (#31223)
  Fully encapsulate LocalCheckpointTracker inside of the engine (#31213)
  Remove version from license file name for GCS SDK (#31221)
  Remove DocumentFieldMappers#simpleMatchToFullName. (#31041)
  [DOCS] Removes 6.3.1 release notes
  [DOCS] Splits release notes by major version
  Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018)
  Remove extraneous references to 'tokenized' in the mapper code. (#31010)
  SQL: Make a single JDBC driver jar (#31012)
  QA: Fix rolling restart tests some more
  Allow to trim all ops above a certain seq# with a term lower than X
  high level REST api: cancel task (#30745)
  Mute TokenBackwardsCompatibilityIT.testMixedCluster
  Mute WatchBackwardsCompatibilityIT.testWatcherRestart
  Enhance license detection for various licenses (#31198)
  [DOCS] Add note about long-lived idle connections (#30990)
  Add high-level client methods that accept RequestOptions (#31069)
  Remove RestGetAllMappingsAction (#31129)
  Move RestGetSettingsAction to RestToXContentListener (#31101)
  Move number of language analyzers to analysis-common module (#31143)
  flush job to ensure all results have been written (#31187)
dnhatn added a commit that referenced this pull request Jun 10, 2018
@dnhatn
CCR side of #31213
97743f3 
Relates #31213
dnhatn added a commit that referenced this pull request Jun 10, 2018
@dnhatn
Merge branch 'master' into ccr
afe02a3 
* master:
  Move default location of dependencies report (#31228)
  Remove dependencies report task dependencies (#31227)
  Add recognition of MPL 2.0 (#31226)
  Fix unknown licenses (#31223)
  Remove version from license file name for GCS SDK (#31221)
  Fully encapsulate LocalCheckpointTracker inside of the engine (#31213)
  [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160)
  Add licenses for transport-nio (#31218)
  Remove DocumentFieldMappers#simpleMatchToFullName. (#31041)
  Allow to trim all ops above a certain seq# with a term lower than X, post backport fix (#31211)
  Compliant SAML Response destination check (#31175)
  Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018)
  Remove extraneous references to 'tokenized' in the mapper code. (#31010)
  Allow to trim all ops above a certain seq# with a term lower than X (#30176)
  SQL: Make a single JDBC driver jar (#31012)
  Enhance license detection for various licenses (#31198)
  [DOCS] Add note about long-lived idle connections (#30990)
  Move number of language analyzers to analysis-common module (#31143)
  Default max concurrent search req. numNodes * 5 (#31171)
  flush job to ensure all results have been written (#31187)
dnhatn added a commit that referenced this pull request Jun 10, 2018
@dnhatn
CCR side of #31213
9feff98 
Relates #31213
jasontedor added a commit to rjernst/elasticsearch that referenced this pull request Jun 10, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into isolate_version_ch…
e332600 
…ecker

* elastic/master: (309 commits)
  [test] add fix for rare virtualbox error (elastic#31212)
  Move default location of dependencies report (elastic#31228)
  Remove dependencies report task dependencies (elastic#31227)
  Add recognition of MPL 2.0 (elastic#31226)
  Fix unknown licenses (elastic#31223)
  Remove version from license file name for GCS SDK (elastic#31221)
  Fully encapsulate LocalCheckpointTracker inside of the engine (elastic#31213)
  [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes elastic#28008 (elastic#31160)
  Add licenses for transport-nio (elastic#31218)
  Remove DocumentFieldMappers#simpleMatchToFullName. (elastic#31041)
  Allow to trim all ops above a certain seq# with a term lower than X, post backport fix (elastic#31211)
  Compliant SAML Response destination check (elastic#31175)
  Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (elastic#31018)
  Remove extraneous references to 'tokenized' in the mapper code. (elastic#31010)
  Allow to trim all ops above a certain seq# with a term lower than X (elastic#30176)
  SQL: Make a single JDBC driver jar (elastic#31012)
  Enhance license detection for various licenses (elastic#31198)
  [DOCS] Add note about long-lived idle connections (elastic#30990)
  Move number of language analyzers to analysis-common module (elastic#31143)
  Default max concurrent search req. numNodes * 5 (elastic#31171)
  ...
@dakrone dakrone deleted the encapsulate-local-checkpoint-tracker branch February 4, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bleskes bleskes bleskes approved these changes

Assignees
No one assigned
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >non-issue v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dakrone @elasticmachine @bleskes @colings86