Skip to content

Commit 7ddab02

Browse files
bleskesbleskes
committed
Allow 0 as a valid external version
Until now all version types have officially required the version to be a positive long number. Despite of this has being documented, ES versions <=1.0 did not enforce it when using the `external` version type. As a result people have succesfully indexed documents with 0 as a version. In 1.1. we introduced validation checks on incoming version values and causing indexing request to fail if the version was set to 0. While this is strictly speaking OK, we effectively have a situation where data already indexed does not match the version invariant. To be lenient and adhere to spirit of our data backward compatibility policy, we have decided to allow 0 as a valid external version type. This is somewhat complicated as 0 is also the internal value of `MATCH_ANY`, which indicates requests should succeed regardles off the current doc version. To keep things simple, this commit changes the internal value of `MATCH_ANY` to `-3` for all version types. Since we're doing this in a minor release (and because versions are stored in the transaction log), the default `internal` version type still accepts 0 as a `MATCH_ANY` value. This is not a problem for other version types as `MATCH_ANY` doesn't make sense in that context. Closes elastic#5662
1 parent 240a2a8 commit 7ddab02

File tree

10 files changed

+100
-25
lines changed

10 files changed

+100
-25
lines changed

‎docs/reference/docs/index_.asciidoc

+4-3
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ By default, internal versioning is used that starts at 1 and increments
109109
with each update, deletes included. Optionally, the version number can be
110110
supplemented with an external value (for example, if maintained in a
111111
database). To enable this functionality, `version_type` should be set to
112-
`external`. The value provided must be a numeric, long value greater than 0,
112+
`external`. The value provided must be a numeric, long value greater or equal to 0,
113113
and less than around 9.2e+18. When using the external version type, instead
114114
of checking for a matching version number, the system checks to see if
115115
the version number passed to the index request is greater than the
@@ -138,12 +138,13 @@ of the stored document.
138138

139139
`external` or `external_gt`:: only index the document if the given version is strictly higher
140140
than the version of the stored document *or* if there is no existing document. The given
141-
version will be used as the new version and will be stored with the new document.
141+
version will be used as the new version and will be stored with the new document. The supplied
142+
version must be a non-negative long number.
142143

143144
`external_gte`:: only index the document if the given version is *equal* or higher
144145
than the version of the stored document. If there is no existing document
145146
the operation will succeed as well. The given version will be used as the new version
146-
and will be stored with the new document.
147+
and will be stored with the new document. The supplied version must be a non-negative long number.
147148

148149
`force`:: the document will be indexed regardless of the version of the stored document or if there
149150
is no existing document. The given version will be used as the new version and will be stored

‎rest-api-spec/test/index/35_external_version.yaml

+22-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,17 @@
11
---
22
"External version":
33

4+
- do:
5+
index:
6+
index: test_1
7+
type: test
8+
id: 1
9+
body: { foo: bar }
10+
version_type: external
11+
version: 0
12+
13+
- match: { _version: 0 }
14+
415
- do:
516
index:
617
index: test_1
@@ -10,7 +21,7 @@
1021
version_type: external
1122
version: 5
1223

13-
- match: { _version: 5}
24+
- match: { _version: 5 }
1425

1526
- do:
1627
catch: conflict
@@ -22,6 +33,16 @@
2233
version_type: external
2334
version: 5
2435

36+
- do:
37+
catch: conflict
38+
index:
39+
index: test_1
40+
type: test
41+
id: 1
42+
body: { foo: bar }
43+
version_type: external
44+
version: 0
45+
2546
- do:
2647
index:
2748
index: test_1

‎rest-api-spec/test/index/36_external_gte_version.yaml

+12-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,17 @@
11
---
22
"External GTE version":
33

4+
- do:
5+
index:
6+
index: test_1
7+
type: test
8+
id: 1
9+
body: { foo: bar }
10+
version_type: external_gte
11+
version: 0
12+
13+
- match: { _version: 0}
14+
415
- do:
516
index:
617
index: test_1
@@ -20,7 +31,7 @@
2031
id: 1
2132
body: { foo: bar }
2233
version_type: external_gte
23-
version: 4
34+
version: 0
2435

2536
- do:
2637
index:

‎src/main/java/org/elasticsearch/action/get/GetRequest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ public void readFrom(StreamInput in) throws IOException {
267267
}
268268

269269
this.versionType = VersionType.fromValue(in.readByte());
270-
this.version = in.readVLong();
270+
this.version = Versions.readFromVLong(in);
271271

272272
fetchSourceContext = FetchSourceContext.optionalReadFromStream(in);
273273
}
@@ -298,7 +298,7 @@ public void writeTo(StreamOutput out) throws IOException {
298298
}
299299

300300
out.writeByte(versionType.getValue());
301-
out.writeVLong(version);
301+
Versions.writeAsVLong(version, out);
302302

303303
FetchSourceContext.optionalWriteToStream(fetchSourceContext, out);
304304
}

‎src/main/java/org/elasticsearch/action/get/MultiGetRequest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ public void readFrom(StreamInput in) throws IOException {
169169
fields[i] = in.readString();
170170
}
171171
}
172-
version = in.readVLong();
172+
version = Versions.readFromVLong(in);
173173
versionType = VersionType.fromValue(in.readByte());
174174

175175
fetchSourceContext = FetchSourceContext.optionalReadFromStream(in);
@@ -190,7 +190,7 @@ public void writeTo(StreamOutput out) throws IOException {
190190
}
191191
}
192192

193-
out.writeVLong(version);
193+
Versions.writeAsVLong(version, out);
194194
out.writeByte(versionType.getValue());
195195

196196
FetchSourceContext.optionalWriteToStream(fetchSourceContext, out);

‎src/main/java/org/elasticsearch/action/get/MultiGetShardRequest.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.elasticsearch.common.Nullable;
2626
import org.elasticsearch.common.io.stream.StreamInput;
2727
import org.elasticsearch.common.io.stream.StreamOutput;
28+
import org.elasticsearch.common.lucene.uid.Versions;
2829
import org.elasticsearch.index.VersionType;
2930
import org.elasticsearch.search.fetch.source.FetchSourceContext;
3031

@@ -138,7 +139,7 @@ public void readFrom(StreamInput in) throws IOException {
138139
} else {
139140
fields.add(null);
140141
}
141-
versions.add(in.readVLong());
142+
versions.add(Versions.readFromVLong(in));
142143
versionTypes.add(VersionType.fromValue(in.readByte()));
143144

144145
fetchSourceContexts.add(FetchSourceContext.optionalReadFromStream(in));
@@ -175,7 +176,7 @@ public void writeTo(StreamOutput out) throws IOException {
175176
out.writeString(field);
176177
}
177178
}
178-
out.writeVLong(versions.get(i));
179+
Versions.writeAsVLong(versions.get(i), out);
179180
out.writeByte(versionTypes.get(i).getValue());
180181
FetchSourceContext fetchSourceContext = fetchSourceContexts.get(i);
181182
FetchSourceContext.optionalWriteToStream(fetchSourceContext, out);

‎src/main/java/org/elasticsearch/common/lucene/uid/Versions.java

+28-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@
2222
import org.apache.lucene.index.*;
2323
import org.apache.lucene.util.Bits;
2424
import org.apache.lucene.util.BytesRef;
25+
import org.elasticsearch.Version;
2526
import org.elasticsearch.common.Numbers;
27+
import org.elasticsearch.common.io.stream.StreamInput;
28+
import org.elasticsearch.common.io.stream.StreamOutput;
2629
import org.elasticsearch.index.mapper.internal.UidFieldMapper;
2730
import org.elasticsearch.index.mapper.internal.VersionFieldMapper;
2831

@@ -32,11 +35,34 @@
3235
/** Utility class to resolve the Lucene doc ID and version for a given uid. */
3336
public class Versions {
3437

35-
public static final long MATCH_ANY = 0L; // Version was not specified by the user
38+
public static final long MATCH_ANY = -3L; // Version was not specified by the user
39+
// the value for MATCH_ANY before ES 1.2.0 - will be removed
40+
public static final long MATCH_ANY_PRE_1_2_0 = 0L;
3641
public static final long NOT_FOUND = -1L;
3742
public static final long NOT_SET = -2L;
3843

39-
private Versions() {}
44+
// The minimum possible for a version value. This is needed to serialize version as positive vLong.
45+
private static final long MIN_VALID_VALUE = -3L;
46+
47+
48+
public static void writeAsVLong(long version, StreamOutput out) throws IOException {
49+
if (out.getVersion().onOrAfter(Version.V_1_2_0)) {
50+
out.writeVLong(version - Versions.MIN_VALID_VALUE);
51+
} else {
52+
out.writeVLong(version);
53+
}
54+
}
55+
56+
public static long readFromVLong(StreamInput in) throws IOException {
57+
if (in.getVersion().onOrAfter(Version.V_1_2_0)) {
58+
return in.readVLong() + Versions.MIN_VALID_VALUE;
59+
} else {
60+
return in.readVLong();
61+
}
62+
}
63+
64+
private Versions() {
65+
}
4066

4167
/** Wraps an {@link AtomicReaderContext}, a doc ID <b>relative to the context doc base</b> and a version. */
4268
public static class DocIdAndVersion {

‎src/main/java/org/elasticsearch/index/VersionType.java

+11-9
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,9 @@ private boolean isVersionConflict(long currentVersion, long expectedVersion) {
4040
if (currentVersion == Versions.NOT_SET) {
4141
return false;
4242
}
43-
if (expectedVersion == Versions.MATCH_ANY) {
43+
// we need to allow pre 1.2.0 match any as requests can come in from older nodes and data may be stored in
44+
// a transaction log.
45+
if (expectedVersion == Versions.MATCH_ANY || expectedVersion == Versions.MATCH_ANY_PRE_1_2_0) {
4446
return false;
4547
}
4648
if (currentVersion == Versions.NOT_FOUND) {
@@ -60,13 +62,13 @@ public long updateVersion(long currentVersion, long expectedVersion) {
6062
@Override
6163
public boolean validateVersionForWrites(long version) {
6264
// not allowing Versions.NOT_FOUND as it is not a valid input value.
63-
return version > 0L || version == Versions.MATCH_ANY;
65+
return version > 0L || version == Versions.MATCH_ANY || version == Versions.MATCH_ANY_PRE_1_2_0;
6466
}
6567

6668
@Override
6769
public boolean validateVersionForReads(long version) {
6870
// not allowing Versions.NOT_FOUND as it is not a valid input value.
69-
return version > 0L || version == Versions.MATCH_ANY;
71+
return version > 0L || version == Versions.MATCH_ANY || version == Versions.MATCH_ANY_PRE_1_2_0;
7072
}
7173

7274
@Override
@@ -118,12 +120,12 @@ public long updateVersion(long currentVersion, long expectedVersion) {
118120

119121
@Override
120122
public boolean validateVersionForWrites(long version) {
121-
return version > 0L;
123+
return version >= 0L;
122124
}
123125

124126
@Override
125127
public boolean validateVersionForReads(long version) {
126-
return version > 0L || version == Versions.MATCH_ANY;
128+
return version >= 0L || version == Versions.MATCH_ANY;
127129
}
128130

129131
},
@@ -169,12 +171,12 @@ public long updateVersion(long currentVersion, long expectedVersion) {
169171

170172
@Override
171173
public boolean validateVersionForWrites(long version) {
172-
return version > 0L;
174+
return version >= 0L;
173175
}
174176

175177
@Override
176178
public boolean validateVersionForReads(long version) {
177-
return version > 0L || version == Versions.MATCH_ANY;
179+
return version >= 0L || version == Versions.MATCH_ANY;
178180
}
179181

180182
},
@@ -208,12 +210,12 @@ public long updateVersion(long currentVersion, long expectedVersion) {
208210

209211
@Override
210212
public boolean validateVersionForWrites(long version) {
211-
return version > 0L;
213+
return version >= 0L;
212214
}
213215

214216
@Override
215217
public boolean validateVersionForReads(long version) {
216-
return version > 0L || version == Versions.MATCH_ANY;
218+
return version >= 0L || version == Versions.MATCH_ANY;
217219
}
218220

219221
};

‎src/main/java/org/elasticsearch/rest/action/support/RestActions.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.elasticsearch.action.support.broadcast.BroadcastOperationResponse;
2626
import org.elasticsearch.common.bytes.BytesArray;
2727
import org.elasticsearch.common.bytes.BytesReference;
28+
import org.elasticsearch.common.lucene.uid.Versions;
2829
import org.elasticsearch.common.xcontent.XContentBuilder;
2930
import org.elasticsearch.common.xcontent.XContentBuilderString;
3031
import org.elasticsearch.index.query.QueryBuilders;
@@ -40,13 +41,13 @@ public class RestActions {
4041

4142
public static long parseVersion(RestRequest request) {
4243
if (request.hasParam("version")) {
43-
return request.paramAsLong("version", 0);
44+
return request.paramAsLong("version", Versions.MATCH_ANY);
4445
}
4546
String ifMatch = request.header("If-Match");
4647
if (ifMatch != null) {
4748
return Long.parseLong(ifMatch);
4849
}
49-
return 0;
50+
return Versions.MATCH_ANY;
5051
}
5152

5253
static final class Fields {

‎src/test/java/org/elasticsearch/index/VersionTypeTests.java

+13-1
Original file line numberDiff line numberDiff line change
@@ -30,19 +30,31 @@ public class VersionTypeTests extends ElasticsearchTestCase {
3030
public void testInternalVersionConflict() throws Exception {
3131

3232
assertFalse(VersionType.INTERNAL.isVersionConflictForWrites(10, Versions.MATCH_ANY));
33+
assertFalse(VersionType.INTERNAL.isVersionConflictForReads(10, Versions.MATCH_ANY));
3334
// if we don't have a version in the index we accept everything
3435
assertFalse(VersionType.INTERNAL.isVersionConflictForWrites(Versions.NOT_SET, 10));
36+
assertFalse(VersionType.INTERNAL.isVersionConflictForReads(Versions.NOT_SET, 10));
3537
assertFalse(VersionType.INTERNAL.isVersionConflictForWrites(Versions.NOT_SET, Versions.MATCH_ANY));
38+
assertFalse(VersionType.INTERNAL.isVersionConflictForReads(Versions.NOT_SET, Versions.MATCH_ANY));
3639

3740
// if we didn't find a version (but the index does support it), we don't like it unless MATCH_ANY
38-
assertTrue(VersionType.INTERNAL.isVersionConflictForWrites(Versions.NOT_FOUND, Versions.NOT_FOUND));
3941
assertTrue(VersionType.INTERNAL.isVersionConflictForWrites(Versions.NOT_FOUND, 10));
42+
assertTrue(VersionType.INTERNAL.isVersionConflictForReads(Versions.NOT_FOUND, 10));
4043
assertFalse(VersionType.INTERNAL.isVersionConflictForWrites(Versions.NOT_FOUND, Versions.MATCH_ANY));
44+
assertFalse(VersionType.INTERNAL.isVersionConflictForReads(Versions.NOT_FOUND, Versions.MATCH_ANY));
45+
46+
// test 0 is still matching any for backwards compatibility with versions <1.2.0
47+
assertFalse(VersionType.INTERNAL.isVersionConflictForWrites(10, Versions.MATCH_ANY_PRE_1_2_0));
48+
assertFalse(VersionType.INTERNAL.isVersionConflictForReads(10, Versions.MATCH_ANY_PRE_1_2_0));
49+
4150

4251
// and the stupid usual case
4352
assertFalse(VersionType.INTERNAL.isVersionConflictForWrites(10, 10));
53+
assertFalse(VersionType.INTERNAL.isVersionConflictForReads(10, 10));
4454
assertTrue(VersionType.INTERNAL.isVersionConflictForWrites(9, 10));
55+
assertTrue(VersionType.INTERNAL.isVersionConflictForReads(9, 10));
4556
assertTrue(VersionType.INTERNAL.isVersionConflictForWrites(10, 9));
57+
assertTrue(VersionType.INTERNAL.isVersionConflictForReads(10, 9));
4658

4759
// Old indexing code, dictating behavior
4860
// if (expectedVersion != Versions.MATCH_ANY && currentVersion != Versions.NOT_SET) {

0 commit comments

Comments
 (0)