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

Add doc's sequence number + primary term to GetResult and use it for updates #36680

Merged
merged 7 commits into from
Dec 17, 2018
Merged

Add doc's sequence number + primary term to GetResult and use it for updates #36680

merged 7 commits into from
Dec 17, 2018

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Dec 16, 2018

This PR adds the last sequence number and primary term of the last operation that have modified a document to GetResult and uses it to power the Update API.

Relates #36148
Relates #10708

@bleskes bleskes added >enhancement :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.0.0 v6.6.0 labels Dec 16, 2018
@bleskes bleskes requested a review from ywelsch December 16, 2018 10:01
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

dnhatn
dnhatn approved these changes Dec 17, 2018
View reviewed changes
Copy link
Member

@dnhatn dnhatn 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 left some minor comments.

...java/org/elasticsearch/xpack/watcher/transport/actions/ack/TransportAckWatchActionTests.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/get/GetResult.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/get/GetResult.java Outdated Show resolved Hide resolved
dnhatn
dnhatn approved these changes Dec 17, 2018
View reviewed changes
Copy link
Member

@dnhatn dnhatn 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 like the way you break into smaller PRs. They are contained and easy to review :).

ywelsch
ywelsch approved these changes Dec 17, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/action/get/GetResponse.java Outdated
}

/**
* The primary term of the last primary that have changed this document, if found.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/have/has/

server/src/main/java/org/elasticsearch/action/update/UpdateResponse.java Outdated
getResult.isExists(),getResult.internalSourceRef(), getResult.getFields()));
update.setGetResult(new GetResult(update.getIndex(), update.getType(), update.getId(),
getResult.getSeqNo(), getResult.getPrimaryTerm(), update.getVersion(),
getResult.isExists(),getResult.internalSourceRef(), getResult.getFields()));
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space

server/src/main/java/org/elasticsearch/action/update/UpdateResponse.java
update.setGetResult(new GetResult(update.getIndex(), update.getType(), update.getId(), update.getVersion(),
getResult.isExists(),getResult.internalSourceRef(), getResult.getFields()));
update.setGetResult(new GetResult(update.getIndex(), update.getType(), update.getId(),
getResult.getSeqNo(), getResult.getPrimaryTerm(), update.getVersion(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use the seq-no and primary term from getResult whereas we use the version from update?

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 saw the too and tried to change it but tests fail. IMO it's a bug but I chose not to fix it in this PR / break BWC.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

server/src/main/java/org/elasticsearch/index/get/GetResult.java
this.index = index;
this.type = type;
this.id = id;
this.seqNo = seqNo;
this.primaryTerm = primaryTerm;
assert (seqNo == SequenceNumbers.UNASSIGNED_SEQ_NO && primaryTerm == 0) || (seqNo >= 0 && primaryTerm >= 1) :
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a new UNASSIGNED_PRIMARY_TERM constant? There are too many of these magic 0 terms sprinkled all over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 here too, for a long time. I don't want to pollute this PR as it will be big and holds to more places. I can do it as a follow up.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok as a follow-up

@bleskes bleskes merged commit e356b8c into elastic:master Dec 17, 2018
@bleskes bleskes deleted the cas_seq_no_update branch December 17, 2018 14:22
@bleskes
Copy link
Contributor Author

bleskes commented Dec 17, 2018

Thanks @ywelsch @dnhatn

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 18, 2018
@jasontedor
Merge branch 'master' into lazier-ingest-geoip
13a87af 
* master: (30 commits)
  Revert "[Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (elastic#35320)"
  Deprecate types in get_source and exist_source (elastic#36426)
  Fix duplicate phrase in shrink/split error message (elastic#36734)
  ingest: support default pipelines + bulk upserts (elastic#36618)
  TESTS:Debug Log. IndexStatsIT#testFilterCacheStats
  [Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (elastic#35320)
  [TEST] fix float comparison in RandomObjects#getExpectedParsedValue
  Initialize startup `CcrRepositories` (elastic#36730)
  ingest: fix on_failure with Drop processor (elastic#36686)
  SNAPSHOTS: Adjust BwC Versions in Restore Logic (elastic#36718)
  [Painless] Add boxed type to boxed type casts for method/return (elastic#36571)
  Do not resolve addresses in remote connection info (elastic#36671)
  Add back one line removed by mistake regarding java version check and COMPAT jvm parameter existence
  Fixing line length for EnvironmentTests and RecoveryTests (elastic#36657)
  SQL: Fix translation of LIKE/RLIKE keywords (elastic#36672)
  [DOCS] Adds monitoring requirement for ingest node (elastic#36665)
  SNAPSHOTS: Disable BwC Tests Until elastic#36659 Landed (elastic#36709)
  Add doc's sequence number + primary term to GetResult and use it for updates (elastic#36680)
  [CCR] Add time since last auto follow fetch to auto follow stats (elastic#36542)
  Watcher accounts constructed lazily (elastic#36656)
  ...
bleskes added a commit that referenced this pull request Dec 18, 2018
@bleskes
Add doc's sequence number + primary term to GetResult and use it for …
9439dbc 
…updates (#36680)

This commit adds the last sequence number and primary term of the last operation that have
modified a document to `GetResult` and uses it to power the Update API.

Relates #36148
Relates #10708
@codebrain codebrain mentioned this pull request May 24, 2019
Merged
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

@dnhatn dnhatn dnhatn approved these changes

Assignees
No one assigned
Labels
:Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >enhancement v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

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