Skip to content

Commit 2d471ce

Browse files
DaveCTurnerDaveCTurner
authored
Treat writtenBy as an opaque string (#74125)
Today `StoreFileMetadata#writtenBy` is a `Version`, but sent over the wire as a `String`. It's included in every file chunk sent during recovery, but parsing it uses `String#replaceFirst` a few times, which is nontrivial, representing >50% of the time it takes to deserialize a `RecoveryFileChunkRequest`. Moreover the value isn't actually used for anything so the parsing is wasted work. With this commit we treat this field as an opaque string, avoiding the unnecessary parsing effort. We don't remove it because it appears in e.g. a snapshot repository, where it may be useful for debugging.
1 parent f6abccc commit 2d471ce

File tree

14 files changed

+82
-58
lines changed

14 files changed

+82
-58
lines changed

‎server/src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,10 @@
99
package org.elasticsearch.index.snapshots.blobstore;
1010

1111
import org.apache.lucene.util.BytesRef;
12-
import org.apache.lucene.util.Version;
1312
import org.elasticsearch.ElasticsearchParseException;
14-
import org.elasticsearch.common.xcontent.ParseField;
1513
import org.elasticsearch.common.Strings;
16-
import org.elasticsearch.common.lucene.Lucene;
1714
import org.elasticsearch.common.unit.ByteSizeValue;
15+
import org.elasticsearch.common.xcontent.ParseField;
1816
import org.elasticsearch.common.xcontent.ToXContentFragment;
1917
import org.elasticsearch.common.xcontent.XContentBuilder;
2018
import org.elasticsearch.common.xcontent.XContentParser;
@@ -266,8 +264,7 @@ public static FileInfo fromXContent(XContentParser parser) throws IOException {
266264
long length = -1;
267265
String checksum = null;
268266
ByteSizeValue partSize = null;
269-
Version writtenBy = null;
270-
String writtenByStr = null;
267+
String writtenBy = null;
271268
BytesRef metaHash = new BytesRef();
272269
XContentParserUtils.ensureExpectedToken(token, XContentParser.Token.START_OBJECT, parser);
273270
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
@@ -286,8 +283,7 @@ public static FileInfo fromXContent(XContentParser parser) throws IOException {
286283
} else if (PART_SIZE.equals(currentFieldName)) {
287284
partSize = new ByteSizeValue(parser.longValue());
288285
} else if (WRITTEN_BY.equals(currentFieldName)) {
289-
writtenByStr = parser.text();
290-
writtenBy = Lucene.parseVersionLenient(writtenByStr, null);
286+
writtenBy = parser.text();
291287
} else if (META_HASH.equals(currentFieldName)) {
292288
metaHash.bytes = parser.binaryValue();
293289
metaHash.offset = 0;
@@ -311,7 +307,7 @@ public static FileInfo fromXContent(XContentParser parser) throws IOException {
311307
} else if (length < 0) {
312308
throw new ElasticsearchParseException("missing or invalid file length");
313309
} else if (writtenBy == null) {
314-
throw new ElasticsearchParseException("missing or invalid written_by [" + writtenByStr + "]");
310+
throw new ElasticsearchParseException("missing or invalid written_by [" + writtenBy + "]");
315311
} else if (checksum == null) {
316312
throw new ElasticsearchParseException("missing checksum for name [" + name + "]");
317313
}

‎server/src/main/java/org/elasticsearch/index/store/Store.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public class Store extends AbstractIndexShardComponent implements Closeable, Ref
126126

127127
/**
128128
* Specific {@link IOContext} indicating that we will read only the Lucene file footer (containing the file checksum)
129-
* See {@link MetadataSnapshot#checksumFromLuceneFile(Directory, String, Map, Logger, Version, boolean)}
129+
* See {@link MetadataSnapshot#checksumFromLuceneFile}.
130130
*/
131131
public static final IOContext READONCE_CHECKSUM = new IOContext(IOContext.READONCE.context);
132132

@@ -835,15 +835,15 @@ static LoadedMetadata loadMetadata(IndexCommit commit, Directory directory, Logg
835835
maxVersion = version;
836836
}
837837
for (String file : info.files()) {
838-
checksumFromLuceneFile(directory, file, builder, logger, version,
838+
checksumFromLuceneFile(directory, file, builder, logger, version.toString(),
839839
SEGMENT_INFO_EXTENSION.equals(IndexFileNames.getExtension(file)));
840840
}
841841
}
842842
if (maxVersion == null) {
843843
maxVersion = org.elasticsearch.Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion;
844844
}
845845
final String segmentsFile = segmentCommitInfos.getSegmentsFileName();
846-
checksumFromLuceneFile(directory, segmentsFile, builder, logger, maxVersion, true);
846+
checksumFromLuceneFile(directory, segmentsFile, builder, logger, maxVersion.toString(), true);
847847
} catch (CorruptIndexException | IndexNotFoundException | IndexFormatTooOldException | IndexFormatTooNewException ex) {
848848
// we either know the index is corrupted or it's just not there
849849
throw ex;
@@ -869,7 +869,7 @@ static LoadedMetadata loadMetadata(IndexCommit commit, Directory directory, Logg
869869
}
870870

871871
private static void checksumFromLuceneFile(Directory directory, String file, Map<String, StoreFileMetadata> builder,
872-
Logger logger, Version version, boolean readFileAsHash) throws IOException {
872+
Logger logger, String version, boolean readFileAsHash) throws IOException {
873873
final String checksum;
874874
final BytesRefBuilder fileHash = new BytesRefBuilder();
875875
try (IndexInput in = directory.openInput(file, READONCE_CHECKSUM)) {

‎server/src/main/java/org/elasticsearch/index/store/StoreFileMetadata.java‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,16 @@ public class StoreFileMetadata implements Writeable {
2929

3030
private final String checksum;
3131

32-
private final Version writtenBy;
32+
private final String writtenBy;
3333

3434
private final BytesRef hash;
3535

36-
public StoreFileMetadata(String name, long length, String checksum, Version writtenBy) {
36+
public StoreFileMetadata(String name, long length, String checksum, String writtenBy) {
3737
this(name, length, checksum, writtenBy, null);
3838
}
3939

40-
public StoreFileMetadata(String name, long length, String checksum, Version writtenBy, BytesRef hash) {
40+
public StoreFileMetadata(String name, long length, String checksum, String writtenBy, BytesRef hash) {
41+
assert assertValidWrittenBy(writtenBy);
4142
this.name = Objects.requireNonNull(name, "name must not be null");
4243
this.length = length;
4344
this.checksum = Objects.requireNonNull(checksum, "checksum must not be null");
@@ -52,11 +53,7 @@ public StoreFileMetadata(StreamInput in) throws IOException {
5253
name = in.readString();
5354
length = in.readVLong();
5455
checksum = in.readString();
55-
try {
56-
writtenBy = Version.parse(in.readString());
57-
} catch (ParseException e) {
58-
throw new AssertionError(e);
59-
}
56+
writtenBy = in.readString();
6057
hash = in.readBytesRef();
6158
}
6259

@@ -65,7 +62,7 @@ public void writeTo(StreamOutput out) throws IOException {
6562
out.writeString(name);
6663
out.writeVLong(length);
6764
out.writeString(checksum);
68-
out.writeString(writtenBy.toString());
65+
out.writeString(writtenBy);
6966
out.writeBytesRef(hash);
7067
}
7168

@@ -127,13 +124,13 @@ public boolean isSame(StoreFileMetadata other) {
127124

128125
@Override
129126
public String toString() {
130-
return "name [" + name + "], length [" + length + "], checksum [" + checksum + "], writtenBy [" + writtenBy + "]" ;
127+
return "name [" + name + "], length [" + length + "], checksum [" + checksum + "], writtenBy [" + writtenBy + "]";
131128
}
132129

133130
/**
134-
* Returns the Lucene version this file has been written by or <code>null</code> if unknown
131+
* Returns a String representation of the Lucene version this file has been written by or <code>null</code> if unknown
135132
*/
136-
public Version writtenBy() {
133+
public String writtenBy() {
137134
return writtenBy;
138135
}
139136

@@ -144,4 +141,13 @@ public Version writtenBy() {
144141
public BytesRef hash() {
145142
return hash;
146143
}
144+
145+
private static boolean assertValidWrittenBy(String writtenBy) {
146+
try {
147+
Version.parse(writtenBy);
148+
} catch (ParseException e) {
149+
throw new AssertionError("invalid writtenBy: " + writtenBy, e);
150+
}
151+
return true;
152+
}
147153
}

‎server/src/main/java/org/elasticsearch/indices/recovery/RecoveryFileChunkRequest.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,9 @@
88

99
package org.elasticsearch.indices.recovery;
1010

11-
import org.apache.lucene.util.Version;
1211
import org.elasticsearch.common.bytes.ReleasableBytesReference;
1312
import org.elasticsearch.common.io.stream.StreamInput;
1413
import org.elasticsearch.common.io.stream.StreamOutput;
15-
import org.elasticsearch.common.lucene.Lucene;
1614
import org.elasticsearch.core.RefCounted;
1715
import org.elasticsearch.index.shard.ShardId;
1816
import org.elasticsearch.index.store.StoreFileMetadata;
@@ -34,13 +32,12 @@ public RecoveryFileChunkRequest(StreamInput in) throws IOException {
3432
super(in);
3533
recoveryId = in.readLong();
3634
shardId = new ShardId(in);
37-
String name = in.readString();
35+
final String name = in.readString();
3836
position = in.readVLong();
39-
long length = in.readVLong();
40-
String checksum = in.readString();
37+
final long length = in.readVLong();
38+
final String checksum = in.readString();
4139
content = in.readReleasableBytesReference();
42-
Version writtenBy = Lucene.parseVersionLenient(in.readString(), null);
43-
assert writtenBy != null;
40+
final String writtenBy = in.readString();
4441
metadata = new StoreFileMetadata(name, length, checksum, writtenBy);
4542
lastChunk = in.readBoolean();
4643
totalTranslogOps = in.readVInt();
@@ -103,7 +100,7 @@ public void writeTo(StreamOutput out) throws IOException {
103100
out.writeVLong(metadata.length());
104101
out.writeString(metadata.checksum());
105102
out.writeBytesReference(content);
106-
out.writeString(metadata.writtenBy().toString());
103+
out.writeString(metadata.writtenBy());
107104
out.writeBoolean(lastChunk);
108105
out.writeVInt(totalTranslogOps);
109106
out.writeLong(sourceThrottleTimeInNanos);

‎server/src/test/java/org/elasticsearch/gateway/ReplicaShardAllocatorTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,8 @@
6363
import static org.hamcrest.Matchers.hasSize;
6464

6565
public class ReplicaShardAllocatorTests extends ESAllocationTestCase {
66-
private static final org.apache.lucene.util.Version MIN_SUPPORTED_LUCENE_VERSION = org.elasticsearch.Version.CURRENT
67-
.minimumIndexCompatibilityVersion().luceneVersion;
66+
private static final String MIN_SUPPORTED_LUCENE_VERSION = Version.CURRENT.minimumIndexCompatibilityVersion().luceneVersion.toString();
67+
6868
private final ShardId shardId = new ShardId("test", "_na_", 0);
6969
private final DiscoveryNode node1 = newNode("node1");
7070
private final DiscoveryNode node2 = newNode("node2");

‎server/src/test/java/org/elasticsearch/index/snapshots/blobstore/FileInfoTests.java‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public void testToFromXContent() throws IOException {
4343
"foobar",
4444
Math.abs(randomLong()),
4545
randomAlphaOfLengthBetween(1, 10),
46-
Version.LATEST,
46+
Version.LATEST.toString(),
4747
hash
4848
);
4949
ByteSizeValue size = new ByteSizeValue(Math.abs(randomLong()));
@@ -64,7 +64,7 @@ public void testToFromXContent() throws IOException {
6464
assertThat(info.partSize(), equalTo(parsedInfo.partSize()));
6565
assertThat(parsedInfo.metadata().hash().length, equalTo(hash.length));
6666
assertThat(parsedInfo.metadata().hash(), equalTo(hash));
67-
assertThat(parsedInfo.metadata().writtenBy(), equalTo(Version.LATEST));
67+
assertThat(parsedInfo.metadata().writtenBy(), equalTo(Version.LATEST.toString()));
6868
assertThat(parsedInfo.isSame(info.metadata()), is(true));
6969
}
7070
}
@@ -123,7 +123,7 @@ public void testInvalidFieldsInFromXContent() throws IOException {
123123
assertThat(length, equalTo(parsedInfo.length()));
124124
assertEquals("666", parsedInfo.checksum());
125125
assertEquals("666", parsedInfo.metadata().checksum());
126-
assertEquals(Version.LATEST, parsedInfo.metadata().writtenBy());
126+
assertEquals(Version.LATEST.toString(), parsedInfo.metadata().writtenBy());
127127
} else {
128128
try (XContentParser parser = createParser(JsonXContent.jsonXContent, xContent)) {
129129
parser.nextToken();
@@ -139,7 +139,7 @@ public void testInvalidFieldsInFromXContent() throws IOException {
139139
public void testGetPartSize() {
140140
BlobStoreIndexShardSnapshot.FileInfo info = new BlobStoreIndexShardSnapshot.FileInfo(
141141
"foo",
142-
new StoreFileMetadata("foo", 36, "666", MIN_SUPPORTED_LUCENE_VERSION),
142+
new StoreFileMetadata("foo", 36, "666", MIN_SUPPORTED_LUCENE_VERSION.toString()),
143143
new ByteSizeValue(6)
144144
);
145145
int numBytes = 0;
@@ -150,7 +150,7 @@ public void testGetPartSize() {
150150

151151
info = new BlobStoreIndexShardSnapshot.FileInfo(
152152
"foo",
153-
new StoreFileMetadata("foo", 35, "666", MIN_SUPPORTED_LUCENE_VERSION),
153+
new StoreFileMetadata("foo", 35, "666", MIN_SUPPORTED_LUCENE_VERSION.toString()),
154154
new ByteSizeValue(6)
155155
);
156156
numBytes = 0;
@@ -160,7 +160,12 @@ public void testGetPartSize() {
160160
assertEquals(numBytes, 35);
161161
final int numIters = randomIntBetween(10, 100);
162162
for (int j = 0; j < numIters; j++) {
163-
StoreFileMetadata metadata = new StoreFileMetadata("foo", randomIntBetween(0, 1000), "666", MIN_SUPPORTED_LUCENE_VERSION);
163+
StoreFileMetadata metadata = new StoreFileMetadata(
164+
"foo",
165+
randomIntBetween(0, 1000),
166+
"666",
167+
MIN_SUPPORTED_LUCENE_VERSION.toString()
168+
);
164169
info = new BlobStoreIndexShardSnapshot.FileInfo("foo", metadata, new ByteSizeValue(randomIntBetween(1, 1000)));
165170
numBytes = 0;
166171
for (int i = 0; i < info.numberOfParts(); i++) {

‎server/src/test/java/org/elasticsearch/index/store/StoreTests.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ public void testVerifyingIndexOutput() throws IOException {
158158
BytesRef ref = new BytesRef(scaledRandomIntBetween(1, 1024));
159159
long length = indexInput.length();
160160
IndexOutput verifyingOutput = new Store.LuceneVerifyingIndexOutput(new StoreFileMetadata("foo1.bar", length, checksum,
161-
MIN_SUPPORTED_LUCENE_VERSION), dir.createOutput("foo1.bar", IOContext.DEFAULT));
161+
MIN_SUPPORTED_LUCENE_VERSION.toString()), dir.createOutput("foo1.bar", IOContext.DEFAULT));
162162
while (length > 0) {
163163
if (random().nextInt(10) == 0) {
164164
verifyingOutput.writeByte(indexInput.readByte());
@@ -191,7 +191,7 @@ public void testVerifyingIndexOutputOnEmptyFile() throws IOException {
191191
Directory dir = newDirectory();
192192
IndexOutput verifyingOutput =
193193
new Store.LuceneVerifyingIndexOutput(new StoreFileMetadata("foo.bar", 0, Store.digestToString(0),
194-
MIN_SUPPORTED_LUCENE_VERSION),
194+
MIN_SUPPORTED_LUCENE_VERSION.toString()),
195195
dir.createOutput("foo1.bar", IOContext.DEFAULT));
196196
try {
197197
Store.verify(verifyingOutput);
@@ -221,7 +221,7 @@ public void testChecksumCorrupted() throws IOException {
221221
BytesRef ref = new BytesRef(scaledRandomIntBetween(1, 1024));
222222
long length = indexInput.length();
223223
IndexOutput verifyingOutput = new Store.LuceneVerifyingIndexOutput(new StoreFileMetadata("foo1.bar", length, checksum,
224-
MIN_SUPPORTED_LUCENE_VERSION), dir.createOutput("foo1.bar", IOContext.DEFAULT));
224+
MIN_SUPPORTED_LUCENE_VERSION.toString()), dir.createOutput("foo1.bar", IOContext.DEFAULT));
225225
length -= 8; // we write the checksum in the try / catch block below
226226
while (length > 0) {
227227
if (random().nextInt(10) == 0) {
@@ -276,7 +276,7 @@ public void testVerifyingIndexOutputWithBogusInput() throws IOException {
276276
Directory dir = newDirectory();
277277
int length = scaledRandomIntBetween(10, 1024);
278278
IndexOutput verifyingOutput = new Store.LuceneVerifyingIndexOutput(new StoreFileMetadata("foo1.bar", length, "",
279-
MIN_SUPPORTED_LUCENE_VERSION), dir.createOutput("foo1.bar", IOContext.DEFAULT));
279+
MIN_SUPPORTED_LUCENE_VERSION.toString()), dir.createOutput("foo1.bar", IOContext.DEFAULT));
280280
try {
281281
while (length > 0) {
282282
verifyingOutput.writeByte((byte) random().nextInt());
@@ -335,7 +335,7 @@ public void testNewChecksums() throws IOException {
335335
try (IndexInput input = store.directory().openInput(meta.name(), IOContext.DEFAULT)) {
336336
String checksum = Store.digestToString(CodecUtil.retrieveChecksum(input));
337337
assertThat("File: " + meta.name() + " has a different checksum", meta.checksum(), equalTo(checksum));
338-
assertThat(meta.writtenBy(), equalTo(Version.LATEST));
338+
assertThat(meta.writtenBy(), equalTo(Version.LATEST.toString()));
339339
if (meta.name().endsWith(".si") || meta.name().startsWith("segments_")) {
340340
assertThat(meta.hash().length, greaterThan(0));
341341
}
@@ -828,9 +828,9 @@ public void testMetadataSnapshotStreaming() throws Exception {
828828

829829
protected Store.MetadataSnapshot createMetadataSnapshot() {
830830
StoreFileMetadata storeFileMetadata1 =
831-
new StoreFileMetadata("segments", 1, "666", MIN_SUPPORTED_LUCENE_VERSION);
831+
new StoreFileMetadata("segments", 1, "666", MIN_SUPPORTED_LUCENE_VERSION.toString());
832832
StoreFileMetadata storeFileMetadata2 =
833-
new StoreFileMetadata("no_segments", 1, "666", MIN_SUPPORTED_LUCENE_VERSION);
833+
new StoreFileMetadata("no_segments", 1, "666", MIN_SUPPORTED_LUCENE_VERSION.toString());
834834
Map<String, StoreFileMetadata> storeFileMetadataMap = new HashMap<>();
835835
storeFileMetadataMap.put(storeFileMetadata1.name(), storeFileMetadata1);
836836
storeFileMetadataMap.put(storeFileMetadata2.name(), storeFileMetadata2);

‎server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,7 @@ private List<StoreFileMetadata> generateFiles(Store store, int numFiles, IntSupp
832832
CRC32 digest = new CRC32();
833833
digest.update(buffer, 0, buffer.length);
834834
StoreFileMetadata md = new StoreFileMetadata("test-" + i, buffer.length + 8,
835-
Store.digestToString(digest.getValue()), org.apache.lucene.util.Version.LATEST);
835+
Store.digestToString(digest.getValue()), org.apache.lucene.util.Version.LATEST.toString());
836836
try (OutputStream out = new IndexOutputOutputStream(store.createVerifyingOutput(md.name(), md, IOContext.DEFAULT))) {
837837
out.write(buffer);
838838
out.write(Numbers.longToBytes(digest.getValue()));

‎server/src/test/java/org/elasticsearch/indices/recovery/RecoveryStatusTests.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,11 @@ public void testRenameTempFiles() throws IOException {
2828
IndexShard indexShard = service.getShardOrNull(0);
2929
MultiFileWriter multiFileWriter = new MultiFileWriter(indexShard.store(),
3030
indexShard.recoveryState().getIndex(), "recovery.test.", logger, () -> {});
31-
try (IndexOutput indexOutput = multiFileWriter.openAndPutIndexOutput("foo.bar",
32-
new StoreFileMetadata("foo.bar", 8 + CodecUtil.footerLength(), "9z51nw", MIN_SUPPORTED_LUCENE_VERSION), indexShard.store())) {
31+
try (IndexOutput indexOutput = multiFileWriter.openAndPutIndexOutput(
32+
"foo.bar",
33+
new StoreFileMetadata("foo.bar", 8 + CodecUtil.footerLength(), "9z51nw", MIN_SUPPORTED_LUCENE_VERSION.toString()),
34+
indexShard.store())) {
35+
3336
indexOutput.writeInt(1);
3437
IndexOutput openIndexOutput = multiFileWriter.getOpenIndexOutput("foo.bar");
3538
assertSame(openIndexOutput, indexOutput);
@@ -38,8 +41,10 @@ public void testRenameTempFiles() throws IOException {
3841
}
3942

4043
try {
41-
multiFileWriter.openAndPutIndexOutput("foo.bar", new StoreFileMetadata("foo.bar", 8 + CodecUtil.footerLength(), "9z51nw",
42-
MIN_SUPPORTED_LUCENE_VERSION), indexShard.store());
44+
multiFileWriter.openAndPutIndexOutput(
45+
"foo.bar",
46+
new StoreFileMetadata("foo.bar", 8 + CodecUtil.footerLength(), "9z51nw", MIN_SUPPORTED_LUCENE_VERSION.toString()),
47+
indexShard.store());
4348
fail("file foo.bar is already opened and registered");
4449
} catch (IllegalStateException ex) {
4550
assertEquals("output for file [foo.bar] has already been created", ex.getMessage());

0 commit comments

Comments
 (0)