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.

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

Allow realtime get to read from translog #48843

Merged

Conversation

@ywelsch
Copy link
Contributor

ywelsch commented Nov 4, 2019

The realtime GET API currently has erratic performance in case where a document is accessed that has just been indexed but not refreshed yet, as the implementation will currently force an internal refresh in that case. Refreshing can be an expensive operation, and also will block the thread that executes the GET operation, blocking other GETs to be processed. In case of frequent access of recently indexed documents, this can lead to a refresh storm and terrible GET performance.

While older versions of Elasticsearch (2.x and older) did not trigger refreshes and instead opted to read from the translog in case of realtime GET API or update API, this was removed in 5.0 (#20102) to avoid inconsistencies between values that were returned from the translog and those returned by the index. This was partially reverted in 6.3 (#29264) to allow _update and upsert to read from the translog again as it was easier to guarantee consistency for these, and also brought back more predictable performance characteristics of this API. Calls to the realtime GET API, however, would still always do a refresh if necessary to return consistent results. This means that users that were calling realtime GET APIs to coordinate updates on client side (realtime GET + CAS for conditional index of updated doc) would still see very erratic performance.

This PR (together with #48707) resolves the inconsistencies between reading from translog and index. In particular it fixes the inconsistencies that happen when requesting stored fields, which were not available when reading from translog. In case where stored fields are requested, this PR will reparse the _source from the translog and derive the stored fields to be returned. With this, it changes the realtime GET API to allow reading from the translog again, avoid refresh storms and blocking the GET threadpool, and provide overall much better and predictable performance for this API.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 4, 2019

Pinging @elastic/es-distributed (:Distributed/Engine)

Copy link
Member

original-brownbear left a comment

LGTM (probably best to wait for one other reviewer though ... I understand this one just fine now but side-effects I'd probably miss :))

ywelsch and others added 4 commits Nov 9, 2019
…dsVisitor.java


more efficient UID decoding

Co-Authored-By: Armin Braun <me@obrown.io>
…ce.java


lighter

Co-Authored-By: Armin Braun <me@obrown.io>
…-when-reading-from-translog
@jpountz
jpountz approved these changes Nov 9, 2019
Copy link
Contributor

jpountz left a comment

Given the number of performance issues I've seen over the past years that were related to refreshes because of GETs, I agree with you that stopping to handle GET requests via the translog was probably a bad idea. We already acknowledged this when re-introducing reads from the translog for updates, I'm good with making all realtime reads use the translog again.

I tried to think about potential sources of inconsistencies and couldn't find any besides of the ones that you already covered.

@henningandersen
Copy link
Contributor

henningandersen commented Nov 9, 2019

Drive-by comment: I wonder if we should attempt to also improve the actual get from translog to always succeed, see:

https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java#L670

I think that this PR together with the PRRL changes to no longer retain additional translog increases the risk of not being able to lookup the operation in translog.

Can certainly be done in a follow-up rather than here.

…GetService.java
" This reverts commit ba4971a.

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch load-stored-fields-when-reading-from-translog
# Changes to be committed:
#	modified:   server/src/main/java/org/elasticsearch/index/get/ShardGetService.java
#
@ywelsch
Copy link
Contributor Author

ywelsch commented Nov 9, 2019

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

I wonder if we should attempt to also improve the actual get from translog to always succeed

I thought a bit about this before pushing this change, and came to the conclusion that it will not matter in practice. Even with peer recovery retention leases now allowing quicker clean-up of translog, this is very unlikely to happen:

  • Retention leases are synced every 30 seconds.
  • Enough translog is kept around to allow restoring from safe commit (typically at least 10s of seconds old).

Triggering an internal refresh in the remaining edge cases does not sound too bad to me.

I can look into what it would take to never fall back to a refresh, but the added complexity might not be worth adding more code to deal with this.

@ywelsch ywelsch merged commit 01030ca into elastic:master Nov 9, 2019
9 checks passed
9 checks passed
CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docs Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample-matrix Build finished.
Details
ywelsch added a commit that referenced this pull request Nov 9, 2019
The realtime GET API currently has erratic performance in case where a document is accessed
that has just been indexed but not refreshed yet, as the implementation will currently force an
internal refresh in that case. Refreshing can be an expensive operation, and also will block the
thread that executes the GET operation, blocking other GETs to be processed. In case of
frequent access of recently indexed documents, this can lead to a refresh storm and terrible
GET performance.

While older versions of Elasticsearch (2.x and older) did not trigger refreshes and instead opted
to read from the translog in case of realtime GET API or update API, this was removed in 5.0
(#20102) to avoid inconsistencies between values that were returned from the translog and
those returned by the index. This was partially reverted in 6.3 (#29264) to allow _update and
upsert to read from the translog again as it was easier to guarantee consistency for these, and
also brought back more predictable performance characteristics of this API. Calls to the realtime
GET API, however, would still always do a refresh if necessary to return consistent results. This
means that users that were calling realtime GET APIs to coordinate updates on client side
(realtime GET + CAS for conditional index of updated doc) would still see very erratic
performance.

This PR (together with #48707) resolves the inconsistencies between reading from translog and
index. In particular it fixes the inconsistencies that happen when requesting stored fields, which
were not available when reading from translog. In case where stored fields are requested, this
PR will reparse the _source from the translog and derive the stored fields to be returned. With
this, it changes the realtime GET API to allow reading from the translog again, avoid refresh
storms and blocking the GET threadpool, and provide overall much better and predictable
performance for this API.
debadair added a commit to debadair/elasticsearch that referenced this pull request Nov 13, 2019
The realtime GET API currently has erratic performance in case where a document is accessed
that has just been indexed but not refreshed yet, as the implementation will currently force an
internal refresh in that case. Refreshing can be an expensive operation, and also will block the
thread that executes the GET operation, blocking other GETs to be processed. In case of
frequent access of recently indexed documents, this can lead to a refresh storm and terrible
GET performance.

While older versions of Elasticsearch (2.x and older) did not trigger refreshes and instead opted
to read from the translog in case of realtime GET API or update API, this was removed in 5.0
(elastic#20102) to avoid inconsistencies between values that were returned from the translog and
those returned by the index. This was partially reverted in 6.3 (elastic#29264) to allow _update and
upsert to read from the translog again as it was easier to guarantee consistency for these, and
also brought back more predictable performance characteristics of this API. Calls to the realtime
GET API, however, would still always do a refresh if necessary to return consistent results. This
means that users that were calling realtime GET APIs to coordinate updates on client side
(realtime GET + CAS for conditional index of updated doc) would still see very erratic
performance.

This PR (together with elastic#48707) resolves the inconsistencies between reading from translog and
index. In particular it fixes the inconsistencies that happen when requesting stored fields, which
were not available when reading from translog. In case where stored fields are requested, this
PR will reparse the _source from the translog and derive the stored fields to be returned. With
this, it changes the realtime GET API to allow reading from the translog again, avoid refresh
storms and blocking the GET threadpool, and provide overall much better and predictable
performance for this API.
@mfussenegger mfussenegger mentioned this pull request Mar 26, 2020
4 of 37 tasks complete
mfussenegger added a commit to crate/crate that referenced this pull request Apr 30, 2020
The only place where we created a `Get` is in `PKLookupOperation` and it
always passed `true` for both, `realtime` and `readFromTranslog`.

That means we can remove the two options and simplifly the handling
within `InternalEngine` a bit.

Encountered this while looking into
elastic/elasticsearch#48843 - a candidate for
backport according to #9796 - but
turns out it doesn't apply to us, as we already allowed reads from
translog.

The `source` inconsistency mentioned in one of the issues also doesn't
apply - we don't expose includes/excludes for source. (And could follow
up on removing the related logic)
mergify bot added a commit to crate/crate that referenced this pull request May 4, 2020
The only place where we created a `Get` is in `PKLookupOperation` and it
always passed `true` for both, `realtime` and `readFromTranslog`.

That means we can remove the two options and simplifly the handling
within `InternalEngine` a bit.

Encountered this while looking into
elastic/elasticsearch#48843 - a candidate for
backport according to #9796 - but
turns out it doesn't apply to us, as we already allowed reads from
translog.

The `source` inconsistency mentioned in one of the issues also doesn't
apply - we don't expose includes/excludes for source. (And could follow
up on removing the related logic)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.