Provide a `getFromTranslog` method in `ShardGetService` by pxsalehi · Pull Request #95736 · elastic/elasticsearch

pxsalehi · 2023-05-02T13:02:36Z

AgetFromTranslog in ShardGetService that shortcuts a get once not found in the translog.
Track the last unsafe segment generation in InternalEngine.

These two together would be used in a TransportGetFromTranslogAction in the real-time get PR. The aim is to return either a result from the index shard's translog, or if not found, return the minimum segment generation that the search shard to wait for before handling the get request locally. Since the index shard might not have a result for the get in its translog due to being in the append-only mode (which the get request would temporarily disable), we need to return the generation where we have switched to a safe live version map.

Relates ES-5999

…mTranslog

pxsalehi · 2023-05-02T13:08:04Z

~~I would like to get some initial feedback if this is how we'd like the approach to look like, before polishing the code or adding tests.~~
This could be potentially broken down into two PRs, if desired.

elasticsearchmachine · 2023-05-03T10:06:19Z

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

pxsalehi

@henningandersen @Tim-Brooks @tlrx This is now cleaned up and I've added test too. Ready for review! :-)

pxsalehi · 2023-05-03T10:08:03Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

        try (ReleasableLock ignored = readLock.acquire()) {
            ensureOpen();
            if (get.realtime()) {
-                final VersionValue versionValue;


All I've done here is to break this down into two pieces. Alternative would be probably a flag to shortcut this, but I'm not sure it makes this more readable. Open for suggestions.

pxsalehi · 2023-05-03T10:10:24Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

-                    // we need to lock here to access the version map to do this truly in RT
-                    versionValue = getVersionFromMap(get.uid().bytes());
+                var result = realtimeGetUnderLock(get, mappingLookup, documentParser, searcherWrapper);
+                if (result != null) {


The normal get, as before, never returns null.

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

…mTranslog

server/src/main/java/org/elasticsearch/index/engine/Engine.java

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

tlrx · 2023-05-03T16:54:24Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

+            assert versionValue.seqNo >= 0 : versionValue;
+            refreshIfNeeded("realtime_get", versionValue.seqNo);
        }
+        return null;


I'd be fine to return an Optional.empty() here and in upstream methods; this would maybe help to distinguish the case when the doc is not found in the translog vs when it's found but deleted?

Optional would be also an option! I just wanted to reduce non-essential changes to the Engine interface since that would change the return type in the upstream methods. The null return value is not affecting the normal Engine.get path. I will add some asserts for it, to make sure. (Alternatively, we could change the return value of get and getFromTranslog to an Optional).

server/src/main/java/org/elasticsearch/index/get/ShardGetService.java

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

server/src/test/java/org/elasticsearch/index/shard/ShardGetServiceTests.java

…mTranslog

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

tlrx · 2023-05-04T11:59:57Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

-                return getFromSearcher(get, acquireSearcher("get", SearcherScope.EXTERNAL, searcherWrapper), false);
            }
+            assert versionValue.seqNo >= 0 : versionValue;
+            refreshIfNeeded("realtime_get", versionValue.seqNo);


Is this refresh needed when the caller is getFromTranslog? I have the impression that we could avoid it but maybe I am missing a point?

I think it is needed since we read from the internal searcher and we need to reload the reader so we can serve the get. The branching is a bit awkward there, I admit. I'll try to improve that.

@tlrx I've pushed 8a973f9. Maybe that makes this clearer.

…gine.java Co-authored-by: Tanguy Leroux <tlrx.dev@gmail.com>

tlrx

LGTM

pxsalehi · 2023-05-05T09:58:30Z

@elasticmachine update branch

henningandersen

LGTM.

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

server/src/test/java/org/elasticsearch/index/shard/ShardGetServiceTests.java

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

…mTranslog

This is a change broken off from the ongoing real-time GET PR (#93976). It is just an action that can be used to invoke the new `ShardGetService.getFromTranslog` in #95736. It will be used on the search shards as a first step to handle a real-time get. Relates #93976, ES-5537

pxsalehi added 3 commits May 2, 2023 14:26

first draft

8b2b501

Merge remote-tracking branch 'upstream/main' into ps230502-sharGetFro…

1d19928

…mTranslog

change

4cede49

pxsalehi added WIP :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. labels May 2, 2023

elasticsearchmachine added the v8.9.0 label May 2, 2023

pxsalehi requested review from henningandersen and tlrx May 2, 2023 13:06

pxsalehi requested a review from Tim-Brooks May 2, 2023 14:55

pxsalehi added 2 commits May 3, 2023 11:57

read from internal searcher only in once case, add test

dfe6754

comment

eb532fd

pxsalehi removed the WIP label May 3, 2023

pxsalehi marked this pull request as ready for review May 3, 2023 10:05

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label May 3, 2023

pxsalehi commented May 3, 2023

View reviewed changes

pxsalehi added the >non-issue label May 3, 2023

Merge remote-tracking branch 'upstream/main' into ps230502-sharGetFro…

1369f07

…mTranslog

tlrx reviewed May 3, 2023

View reviewed changes

address review comments

5f7d6b6

pxsalehi requested a review from tlrx May 4, 2023 09:59

Merge remote-tracking branch 'upstream/main' into ps230502-sharGetFro…

82d09a2

…mTranslog

tlrx reviewed May 4, 2023

View reviewed changes

Update server/src/main/java/org/elasticsearch/index/engine/InternalEn…

a009918

…gine.java Co-authored-by: Tanguy Leroux <tlrx.dev@gmail.com>

pxsalehi requested a review from tlrx May 4, 2023 14:14

comment and simplify

8a973f9

tlrx approved these changes May 4, 2023

View reviewed changes

Merge branch 'main' into ps230502-sharGetFromTranslog

4ed865e

minor test clarifications

b42d930

henningandersen approved these changes May 5, 2023

View reviewed changes

pxsalehi added 3 commits May 8, 2023 13:56

address review comments

2b3d730

Merge remote-tracking branch 'upstream/main' into ps230502-sharGetFro…

fe74dfd

…mTranslog

tiny comment

41c253f

pxsalehi added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 8, 2023

elasticsearchmachine merged commit f02e9f9 into elastic:main May 8, 2023

pxsalehi deleted the ps230502-sharGetFromTranslog branch May 8, 2023 12:53

pxsalehi mentioned this pull request May 10, 2023

Introduce TransportGetFromTranslogAction #95998

Merged

Conversation

pxsalehi commented May 2, 2023

Uh oh!

pxsalehi commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented May 3, 2023

Uh oh!

pxsalehi left a comment

Choose a reason for hiding this comment

Uh oh!

pxsalehi May 3, 2023

Choose a reason for hiding this comment

Uh oh!

pxsalehi May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tlrx May 3, 2023

Choose a reason for hiding this comment

Uh oh!

pxsalehi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tlrx May 4, 2023

Choose a reason for hiding this comment

Uh oh!

pxsalehi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

pxsalehi May 4, 2023

Choose a reason for hiding this comment

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

pxsalehi commented May 5, 2023

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pxsalehi commented May 2, 2023 •

edited

Loading