Skip to content

Commit 2af5de4

Browse files
tlrxtlrx
committed
Rename and remove some ShardGetService get methods
ShardGetService contains multiple get*() methods with different visibility. One `getForUpdate` is only used in a test so it can be moved there. An `innerGet` private method is only called by the private get() method so it can be folded in there to remove one more undocumented `get` variant. A public `get` method is only used in TransportShardMultiGetAction so we can rename it to `mget` to avoid confusion with other `get`. Finally the private `get` is renamed to `doGet` to indicate it's an "inner" private method.
1 parent 27adf20 commit 2af5de4

File tree

3 files changed

+52
-62
lines changed

3 files changed

+52
-62
lines changed

‎server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ private void getAndAddToResponse(
312312
MultiGetRequest.Item item = request.items.get(location);
313313
try {
314314
GetResult getResult = indexShard.getService()
315-
.get(
315+
.mget(
316316
item.id(),
317317
item.storedFields(),
318318
request.realtime(),

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

Lines changed: 38 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public GetResult get(
9393
FetchSourceContext fetchSourceContext,
9494
boolean forceSyntheticSource
9595
) throws IOException {
96-
return get(
96+
return doGet(
9797
id,
9898
gFields,
9999
realtime,
@@ -107,7 +107,7 @@ public GetResult get(
107107
);
108108
}
109109

110-
public GetResult get(
110+
public GetResult mget(
111111
String id,
112112
String[] gFields,
113113
boolean realtime,
@@ -117,7 +117,7 @@ public GetResult get(
117117
boolean forceSyntheticSource,
118118
MultiEngineGet mget
119119
) throws IOException {
120-
return get(
120+
return doGet(
121121
id,
122122
gFields,
123123
realtime,
@@ -131,7 +131,7 @@ public GetResult get(
131131
);
132132
}
133133

134-
private GetResult get(
134+
private GetResult doGet(
135135
String id,
136136
String[] gFields,
137137
boolean realtime,
@@ -144,21 +144,40 @@ private GetResult get(
144144
Function<Engine.Get, Engine.GetResult> engineGetOperator
145145
) throws IOException {
146146
currentMetric.inc();
147+
final long now = System.nanoTime();
147148
try {
148-
long now = System.nanoTime();
149-
GetResult getResult = innerGet(
150-
id,
151-
gFields,
152-
realtime,
153-
version,
154-
versionType,
155-
ifSeqNo,
156-
ifPrimaryTerm,
157-
fetchSourceContext,
158-
forceSyntheticSource,
159-
engineGetOperator
160-
);
149+
var engineGet = new Engine.Get(realtime, realtime, id).version(version)
150+
.versionType(versionType)
151+
.setIfSeqNo(ifSeqNo)
152+
.setIfPrimaryTerm(ifPrimaryTerm);
161153

154+
final GetResult getResult;
155+
try (Engine.GetResult get = engineGetOperator.apply(engineGet)) {
156+
if (get == null) {
157+
getResult = null;
158+
} else if (get.exists() == false) {
159+
getResult = new GetResult(
160+
shardId.getIndexName(),
161+
id,
162+
UNASSIGNED_SEQ_NO,
163+
UNASSIGNED_PRIMARY_TERM,
164+
-1,
165+
false,
166+
null,
167+
null,
168+
null
169+
);
170+
} else {
171+
// break between having loaded it from translog (so we only have _source), and having a document to load
172+
getResult = innerGetFetch(
173+
id,
174+
gFields,
175+
normalizeFetchSourceContent(fetchSourceContext, gFields),
176+
get,
177+
forceSyntheticSource
178+
);
179+
}
180+
}
162181
if (getResult != null && getResult.isExists()) {
163182
existsMetric.inc(System.nanoTime() - now);
164183
} else {
@@ -179,7 +198,7 @@ public GetResult getFromTranslog(
179198
FetchSourceContext fetchSourceContext,
180199
boolean forceSyntheticSource
181200
) throws IOException {
182-
return get(
201+
return doGet(
183202
id,
184203
gFields,
185204
realtime,
@@ -193,12 +212,8 @@ public GetResult getFromTranslog(
193212
);
194213
}
195214

196-
public GetResult getForUpdate(String id, long ifSeqNo, long ifPrimaryTerm) throws IOException {
197-
return getForUpdate(id, ifSeqNo, ifPrimaryTerm, new String[] { RoutingFieldMapper.NAME });
198-
}
199-
200215
public GetResult getForUpdate(String id, long ifSeqNo, long ifPrimaryTerm, String[] gFields) throws IOException {
201-
return get(
216+
return doGet(
202217
id,
203218
gFields,
204219
true,
@@ -259,35 +274,6 @@ private static FetchSourceContext normalizeFetchSourceContent(@Nullable FetchSou
259274
return FetchSourceContext.DO_NOT_FETCH_SOURCE;
260275
}
261276

262-
private GetResult innerGet(
263-
String id,
264-
String[] gFields,
265-
boolean realtime,
266-
long version,
267-
VersionType versionType,
268-
long ifSeqNo,
269-
long ifPrimaryTerm,
270-
FetchSourceContext fetchSourceContext,
271-
boolean forceSyntheticSource,
272-
Function<Engine.Get, Engine.GetResult> engineGetOperator
273-
) throws IOException {
274-
fetchSourceContext = normalizeFetchSourceContent(fetchSourceContext, gFields);
275-
var engineGet = new Engine.Get(realtime, realtime, id).version(version)
276-
.versionType(versionType)
277-
.setIfSeqNo(ifSeqNo)
278-
.setIfPrimaryTerm(ifPrimaryTerm);
279-
try (Engine.GetResult get = engineGetOperator.apply(engineGet)) {
280-
if (get == null) {
281-
return null;
282-
}
283-
if (get.exists() == false) {
284-
return new GetResult(shardId.getIndexName(), id, UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM, -1, false, null, null, null);
285-
}
286-
// break between having loaded it from translog (so we only have _source), and having a document to load
287-
return innerGetFetch(id, gFields, fetchSourceContext, get, forceSyntheticSource);
288-
}
289-
}
290-
291277
private GetResult innerGetFetch(
292278
String id,
293279
String[] storedFields,

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

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@
3434

3535
public class ShardGetServiceTests extends IndexShardTestCase {
3636

37+
private GetResult getForUpdate(IndexShard indexShard, String id, long ifSeqNo, long ifPrimaryTerm) throws IOException {
38+
return indexShard.getService().getForUpdate(id, ifSeqNo, ifPrimaryTerm, new String[] { RoutingFieldMapper.NAME });
39+
}
40+
3741
public void testGetForUpdate() throws IOException {
3842
Settings settings = indexSettings(IndexVersion.current(), 1, 1).build();
3943
IndexMetadata metadata = IndexMetadata.builder("test").putMapping("""
@@ -44,7 +48,7 @@ public void testGetForUpdate() throws IOException {
4448
long translogInMemorySegmentCountExpected = 0;
4549
Engine.IndexResult test = indexDoc(primary, "test", "0", "{\"foo\" : \"bar\"}");
4650
assertTrue(primary.getEngine().refreshNeeded());
47-
GetResult testGet = primary.getService().getForUpdate("0", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
51+
GetResult testGet = getForUpdate(primary, "0", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
4852
assertFalse(testGet.getFields().containsKey(RoutingFieldMapper.NAME));
4953
assertEquals(testGet.sourceRef().utf8ToString(), "{\"foo\" : \"bar\"}");
5054
assertEquals(translogInMemorySegmentCountExpected, translogInMemorySegmentCount.getAsLong());
@@ -54,7 +58,7 @@ public void testGetForUpdate() throws IOException {
5458

5559
Engine.IndexResult test1 = indexDoc(primary, "1", "{\"foo\" : \"baz\"}", XContentType.JSON, "foobar");
5660
assertTrue(primary.getEngine().refreshNeeded());
57-
GetResult testGet1 = primary.getService().getForUpdate("1", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
61+
GetResult testGet1 = getForUpdate(primary, "1", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
5862
assertEquals(testGet1.sourceRef().utf8ToString(), "{\"foo\" : \"baz\"}");
5963
assertTrue(testGet1.getFields().containsKey(RoutingFieldMapper.NAME));
6064
assertEquals("foobar", testGet1.getFields().get(RoutingFieldMapper.NAME).getValue());
@@ -70,19 +74,19 @@ public void testGetForUpdate() throws IOException {
7074
// now again from the reader
7175
Engine.IndexResult test2 = indexDoc(primary, "1", "{\"foo\" : \"baz\"}", XContentType.JSON, "foobar");
7276
assertTrue(primary.getEngine().refreshNeeded());
73-
testGet1 = primary.getService().getForUpdate("1", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
77+
testGet1 = getForUpdate(primary, "1", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
7478
assertEquals(testGet1.sourceRef().utf8ToString(), "{\"foo\" : \"baz\"}");
7579
assertTrue(testGet1.getFields().containsKey(RoutingFieldMapper.NAME));
7680
assertEquals("foobar", testGet1.getFields().get(RoutingFieldMapper.NAME).getValue());
7781
assertEquals(translogInMemorySegmentCountExpected, translogInMemorySegmentCount.getAsLong());
7882

7983
final long primaryTerm = primary.getOperationPrimaryTerm();
80-
testGet1 = primary.getService().getForUpdate("1", test2.getSeqNo(), primaryTerm);
84+
testGet1 = getForUpdate(primary, "1", test2.getSeqNo(), primaryTerm);
8185
assertEquals(testGet1.sourceRef().utf8ToString(), "{\"foo\" : \"baz\"}");
8286
assertEquals(translogInMemorySegmentCountExpected, translogInMemorySegmentCount.getAsLong());
8387

84-
expectThrows(VersionConflictEngineException.class, () -> primary.getService().getForUpdate("1", test2.getSeqNo() + 1, primaryTerm));
85-
expectThrows(VersionConflictEngineException.class, () -> primary.getService().getForUpdate("1", test2.getSeqNo(), primaryTerm + 1));
88+
expectThrows(VersionConflictEngineException.class, () -> getForUpdate(primary, "1", test2.getSeqNo() + 1, primaryTerm));
89+
expectThrows(VersionConflictEngineException.class, () -> getForUpdate(primary, "1", test2.getSeqNo(), primaryTerm + 1));
8690
closeShards(primary);
8791
}
8892

@@ -183,7 +187,7 @@ private void runGetFromTranslogWithOptions(
183187
Engine.IndexResult res = indexDoc(primary, "test", "0", docToIndex);
184188
assertTrue(res.isCreated());
185189
assertTrue(primary.getEngine().refreshNeeded());
186-
GetResult testGet = primary.getService().getForUpdate("0", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
190+
GetResult testGet = getForUpdate(primary, "0", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
187191
assertFalse(testGet.getFields().containsKey(RoutingFieldMapper.NAME));
188192
assertFalse(testGet.getFields().containsKey("foo"));
189193
assertFalse(testGet.getFields().containsKey("bar"));
@@ -194,7 +198,7 @@ private void runGetFromTranslogWithOptions(
194198

195199
indexDoc(primary, "1", docToIndex, XContentType.JSON, "foobar");
196200
assertTrue(primary.getEngine().refreshNeeded());
197-
GetResult testGet1 = primary.getService().getForUpdate("1", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
201+
GetResult testGet1 = getForUpdate(primary, "1", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
198202
assertEquals(testGet1.sourceRef() == null ? "" : testGet1.sourceRef().utf8ToString(), expectedResult);
199203
assertTrue(testGet1.getFields().containsKey(RoutingFieldMapper.NAME));
200204
assertFalse(testGet.getFields().containsKey("foo"));
@@ -252,7 +256,7 @@ public void testTypelessGetForUpdate() throws IOException {
252256
Engine.IndexResult indexResult = indexDoc(shard, "some_type", "0", "{\"foo\" : \"bar\"}");
253257
assertTrue(indexResult.isCreated());
254258

255-
GetResult getResult = shard.getService().getForUpdate("0", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
259+
GetResult getResult = getForUpdate(shard, "0", UNASSIGNED_SEQ_NO, UNASSIGNED_PRIMARY_TERM);
256260
assertTrue(getResult.isExists());
257261

258262
closeShards(shard);

0 commit comments

Comments
 (0)