address review comments

Author: pxsalehi

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

@@ -634,8 +634,10 @@ public abstract GetResult get(
         Function<Engine.Searcher, Engine.Searcher> searcherWrapper
     );
 
-    // Similar to Engine.get, but it only attempts to serve the get from the translog.
-    // If not found in translog, it returns null, as {@code GetResult.NOT_EXISTS} could mean deletion.
+    /**
+     * Similar to {@link Engine#get}, but it only attempts to serve the get from the translog.
+     * If not found in translog, it returns null, as {@link GetResult#NOT_EXISTS} could mean deletion.
+     */
     public GetResult getFromTranslog(
         Get get,
         MappingLookup mappingLookup,

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

@@ -142,7 +142,7 @@ public class InternalEngine extends Engine {
     // A uid (in the form of BytesRef) to the version map
     // we use the hashed variant since we iterate over it and check removal and additions on existing keys
     private final LiveVersionMap versionMap = new LiveVersionMap();
-    // Records the last known generation during which LiverVersionMap was in unsafe mode. This indicates that only after this
+    // Records the last known generation during which LiveVersionMap was in unsafe mode. This indicates that only after this
     // generation it is safe to rely on the LiveVersionMap for a real-time get.
     private final AtomicLong lastUnsafeSegmentGenerationForGets = new AtomicLong(-1);
 
@@ -753,7 +753,7 @@ public GetResult get(
         DocumentParser documentParser,
         Function<Engine.Searcher, Engine.Searcher> searcherWrapper
     ) {
-        assert Objects.equals(get.uid().field(), IdFieldMapper.NAME) : get.uid().field();
+        assert assertGetUsesIdField(get);
         try (ReleasableLock ignored = readLock.acquire()) {
             ensureOpen();
             if (get.realtime()) {
@@ -776,7 +776,7 @@ public GetResult getFromTranslog(
         DocumentParser documentParser,
         Function<Searcher, Searcher> searcherWrapper
     ) {
-        assert Objects.equals(get.uid().field(), IdFieldMapper.NAME) : get.uid().field();
+        assert assertGetUsesIdField(get);
         try (ReleasableLock ignored = readLock.acquire()) {
             ensureOpen();
             return realtimeGetUnderLock(get, mappingLookup, documentParser, searcherWrapper);
@@ -790,6 +790,7 @@ protected GetResult realtimeGetUnderLock(
         Function<Engine.Searcher, Engine.Searcher> searcherWrapper
     ) {
         assert readLock.isHeldByCurrentThread();
+        assert get.realtime();
         final VersionValue versionValue;
         try (Releasable ignore = versionMap.acquireLock(get.uid().bytes())) {
             // we need to lock here to access the version map to do this truly in RT
@@ -3214,4 +3215,9 @@ public long getLastUnsafeSegmentGenerationForGets() {
     public LiveVersionMap getLiveVersionMap() {
         return versionMap;
     }
+
+    private static boolean assertGetUsesIdField(Get get) {
+        assert Objects.equals(get.uid().field(), IdFieldMapper.NAME) : get.uid().field();
+        return true;
+    }
 }

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

@@ -228,7 +228,11 @@ private GetResult innerGet(
         boolean translogOnly
     ) throws IOException {
         fetchSourceContext = normalizeFetchSourceContent(fetchSourceContext, gFields);
-        try (Engine.GetResult get = getFromIndexShard(id, realtime, version, versionType, ifSeqNo, ifPrimaryTerm, translogOnly)) {
+        var engineGet = new Engine.Get(realtime, realtime, id).version(version)
+            .versionType(versionType)
+            .setIfSeqNo(ifSeqNo)
+            .setIfPrimaryTerm(ifPrimaryTerm);
+        try (Engine.GetResult get = translogOnly ? indexShard.getFromTranslog(engineGet) : indexShard.get(engineGet)) {
             if (get == null) {
                 return null;
             }
@@ -240,22 +244,6 @@ private GetResult innerGet(
         }
     }
 
-    private Engine.GetResult getFromIndexShard(
-        String id,
-        boolean realtime,
-        long version,
-        VersionType versionType,
-        long ifSeqNo,
-        long ifPrimaryTerm,
-        boolean translogOnly
-    ) {
-        var engineGet = new Engine.Get(realtime, realtime, id).version(version)
-            .versionType(versionType)
-            .setIfSeqNo(ifSeqNo)
-            .setIfPrimaryTerm(ifPrimaryTerm);
-        return translogOnly ? indexShard.getFromTranslog(engineGet) : indexShard.get(engineGet);
-    }
-
     private GetResult innerGetFetch(
         String id,
         String[] storedFields,

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -1222,7 +1222,9 @@ private Engine.GetResult innerGet(Engine.Get get, boolean translogOnly) {
         if (translogOnly) {
             return getEngine().getFromTranslog(get, mappingLookup, mapperService.documentParser(), this::wrapSearcher);
         }
-        return getEngine().get(get, mappingLookup, mapperService.documentParser(), this::wrapSearcher);
+        var result = getEngine().get(get, mappingLookup, mapperService.documentParser(), this::wrapSearcher);
+        assert result != null : "result cannot be null";
+        return result;
     }
 
     /**

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

@@ -30,6 +30,7 @@
 import static org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_PRIMARY_TERM;
 import static org.elasticsearch.index.seqno.SequenceNumbers.UNASSIGNED_SEQ_NO;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
 
 public class ShardGetServiceTests extends IndexShardTestCase {
 
@@ -233,7 +234,7 @@ public void testGetFromTranslog() throws IOException {
             .getFromTranslog("2", new String[] { "foo" }, true, 1, VersionType.INTERNAL, FetchSourceContext.FETCH_SOURCE, false);
         assertNull(getResult);
         var lastUnsafeGeneration = engine.getLastUnsafeSegmentGenerationForGets();
-        assertTrue(lastUnsafeGeneration > 0);
+        assertThat(lastUnsafeGeneration, greaterThan(0L));
         assertTrue(LiveVersionMapTestUtils.isSafeAccessRequired(map));
         assertFalse(LiveVersionMapTestUtils.isUnsafe(map));
         // A flush shouldn't change the recorded last unsafe generation for gets