Skip to content

Commit 44a75d9

Browse files
spearcespearce
committed
reftable: explicitly store update_index per ref
Add an update_index to every reference in a reftable, storing the exact transaction that last modified the reference. This is necessary to fix some merge race conditions. Consider updates at T1, T3 are present in two reftables. Compacting these will create a table with range [T1,T3]. If T2 arrives during or after the compaction its impossible for readers to know how to merge the [T1,T3] table with the T2 table. With an explicit update_index per reference, MergedReftable is able to individually sort each reference, merging individual entries at T3 from [T1,T3] ahead of identically named entries appearing in T2. Change-Id: Ie4065d4176a5a0207dcab9696ae05d086e042140
1 parent 231f5d9 commit 44a75d9

File tree

10 files changed

+134
-39
lines changed

10 files changed

+134
-39
lines changed

‎Documentation/technical/reftable.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,7 @@ its value(s). Records are formatted as:
243243
varint( prefix_length )
244244
varint( (suffix_length << 3) | value_type )
245245
suffix
246+
varint( update_index_delta )
246247
value?
247248

248249
The `prefix_length` field specifies how many leading bytes of the
@@ -258,6 +259,10 @@ Recovering a reference name from any `ref_record` is a simple concat:
258259
The `suffix_length` value provides the number of bytes available in
259260
`suffix` to copy from `suffix` to complete the reference name.
260261

262+
The `update_index` that last modified the reference can be obtained by
263+
adding `update_index_delta` to the `min_update_index` from the file
264+
header: `min_update_index + update_index_delta`.
265+
261266
The `value` follows. Its format is determined by `value_type`, one of
262267
the following:
263268

‎org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/MergedReftableTest.java

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,41 @@ public void oneTableSeek() throws IOException {
261261
}
262262
}
263263

264+
@Test
265+
public void missedUpdate() throws IOException {
266+
ByteArrayOutputStream buf = new ByteArrayOutputStream();
267+
ReftableWriter writer = new ReftableWriter()
268+
.setMinUpdateIndex(1)
269+
.setMaxUpdateIndex(3)
270+
.begin(buf);
271+
writer.writeRef(ref("refs/heads/a", 1), 1);
272+
writer.writeRef(ref("refs/heads/c", 3), 3);
273+
writer.finish();
274+
byte[] base = buf.toByteArray();
275+
276+
byte[] delta = write(Arrays.asList(
277+
ref("refs/heads/b", 2),
278+
ref("refs/heads/c", 4)),
279+
2);
280+
MergedReftable mr = merge(base, delta);
281+
try (RefCursor rc = mr.allRefs()) {
282+
assertTrue(rc.next());
283+
assertEquals("refs/heads/a", rc.getRef().getName());
284+
assertEquals(id(1), rc.getRef().getObjectId());
285+
assertEquals(1, rc.getUpdateIndex());
286+
287+
assertTrue(rc.next());
288+
assertEquals("refs/heads/b", rc.getRef().getName());
289+
assertEquals(id(2), rc.getRef().getObjectId());
290+
assertEquals(2, rc.getUpdateIndex());
291+
292+
assertTrue(rc.next());
293+
assertEquals("refs/heads/c", rc.getRef().getName());
294+
assertEquals(id(3), rc.getRef().getObjectId());
295+
assertEquals(3, rc.getUpdateIndex());
296+
}
297+
}
298+
264299
@Test
265300
public void compaction() throws IOException {
266301
List<Ref> delta1 = Arrays.asList(
@@ -322,12 +357,18 @@ private byte[] write(Ref... refs) throws IOException {
322357
}
323358

324359
private byte[] write(Collection<Ref> refs) throws IOException {
360+
return write(refs, 1);
361+
}
362+
363+
private byte[] write(Collection<Ref> refs, long updateIndex)
364+
throws IOException {
325365
ByteArrayOutputStream buffer = new ByteArrayOutputStream();
326-
ReftableWriter writer = new ReftableWriter().begin(buffer);
327-
for (Ref r : RefComparator.sort(refs)) {
328-
writer.writeRef(r);
329-
}
330-
writer.finish();
366+
new ReftableWriter()
367+
.setMinUpdateIndex(updateIndex)
368+
.setMaxUpdateIndex(updateIndex)
369+
.begin(buffer)
370+
.sortAndWriteRefs(refs)
371+
.finish();
331372
return buffer.toByteArray();
332373
}
333374

‎org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public void emptyVirtualTableFromRefs() throws IOException {
126126
@Test
127127
public void estimateCurrentBytesOneRef() throws IOException {
128128
Ref exp = ref(MASTER, 1);
129-
int expBytes = 24 + 4 + 5 + 3 + MASTER.length() + 20 + 68;
129+
int expBytes = 24 + 4 + 5 + 4 + MASTER.length() + 20 + 68;
130130

131131
byte[] table;
132132
ReftableConfig cfg = new ReftableConfig();
@@ -155,7 +155,7 @@ public void estimateCurrentBytesWithIndex() throws IOException {
155155
cfg.setIndexObjects(false);
156156
cfg.setMaxIndexLevels(1);
157157

158-
int expBytes = 139654;
158+
int expBytes = 147860;
159159
byte[] table;
160160
ReftableWriter writer = new ReftableWriter().setConfig(cfg);
161161
try (ByteArrayOutputStream buf = new ByteArrayOutputStream()) {
@@ -174,7 +174,7 @@ public void estimateCurrentBytesWithIndex() throws IOException {
174174
public void oneIdRef() throws IOException {
175175
Ref exp = ref(MASTER, 1);
176176
byte[] table = write(exp);
177-
assertEquals(24 + 4 + 5 + 3 + MASTER.length() + 20 + 68, table.length);
177+
assertEquals(24 + 4 + 5 + 4 + MASTER.length() + 20 + 68, table.length);
178178

179179
ReftableReader t = read(table);
180180
try (RefCursor rc = t.allRefs()) {
@@ -203,7 +203,7 @@ public void oneIdRef() throws IOException {
203203
public void oneTagRef() throws IOException {
204204
Ref exp = tag(V1_0, 1, 2);
205205
byte[] table = write(exp);
206-
assertEquals(24 + 4 + 5 + 2 + V1_0.length() + 40 + 68, table.length);
206+
assertEquals(24 + 4 + 5 + 3 + V1_0.length() + 40 + 68, table.length);
207207

208208
ReftableReader t = read(table);
209209
try (RefCursor rc = t.allRefs()) {
@@ -224,7 +224,7 @@ public void oneSymbolicRef() throws IOException {
224224
Ref exp = sym(HEAD, MASTER);
225225
byte[] table = write(exp);
226226
assertEquals(
227-
24 + 4 + 5 + 2 + HEAD.length() + 1 + MASTER.length() + 68,
227+
24 + 4 + 5 + 2 + HEAD.length() + 2 + MASTER.length() + 68,
228228
table.length);
229229

230230
ReftableReader t = read(table);
@@ -281,7 +281,7 @@ public void oneDeletedRef() throws IOException {
281281
String name = "refs/heads/gone";
282282
Ref exp = newRef(name);
283283
byte[] table = write(exp);
284-
assertEquals(24 + 4 + 5 + 2 + name.length() + 68, table.length);
284+
assertEquals(24 + 4 + 5 + 3 + name.length() + 68, table.length);
285285

286286
ReftableReader t = read(table);
287287
try (RefCursor rc = t.allRefs()) {
@@ -425,13 +425,14 @@ public void withReflog() throws IOException {
425425

426426
writer.finish();
427427
byte[] table = buffer.toByteArray();
428-
assertEquals(245, table.length);
428+
assertEquals(247, table.length);
429429

430430
ReftableReader t = read(table);
431431
try (RefCursor rc = t.allRefs()) {
432432
assertTrue(rc.next());
433433
assertEquals(MASTER, rc.getRef().getName());
434434
assertEquals(id(1), rc.getRef().getObjectId());
435+
assertEquals(1, rc.getUpdateIndex());
435436

436437
assertTrue(rc.next());
437438
assertEquals(NEXT, rc.getRef().getName());
@@ -636,7 +637,7 @@ public void nameTooLongDoesNotWrite() throws IOException {
636637
writer.finish();
637638
fail("expected BlockSizeTooSmallException");
638639
} catch (BlockSizeTooSmallException e) {
639-
assertEquals(84, e.getMinimumBlockSize());
640+
assertEquals(85, e.getMinimumBlockSize());
640641
}
641642
}
642643

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,10 @@ long readPositionFromIndex() throws IOException {
166166
return readVarint64();
167167
}
168168

169+
long readUpdateIndexDelta() {
170+
return readVarint64();
171+
}
172+
169173
Ref readRef() throws IOException {
170174
String name = RawParseUtils.decode(UTF_8, nameBuf, 0, nameLen);
171175
switch (valueType & VALUE_TYPE_MASK) {
@@ -490,6 +494,7 @@ private int scanToKey(byte[] key, int rPtr, int rIdx, int rCmp) {
490494
void skipValue() {
491495
switch (blockType) {
492496
case REF_BLOCK_TYPE:
497+
readVarint64(); // update_index_delta
493498
switch (valueType & VALUE_TYPE_MASK) {
494499
case VALUE_NONE:
495500
return;

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockWriter.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -354,10 +354,12 @@ void writeValue(ReftableOutputStream os) {
354354

355355
static class RefEntry extends Entry {
356356
final Ref ref;
357+
final long updateIndexDelta;
357358

358-
RefEntry(Ref ref) {
359+
RefEntry(Ref ref, long updateIndexDelta) {
359360
super(nameUtf8(ref));
360361
this.ref = ref;
362+
this.updateIndexDelta = updateIndexDelta;
361363
}
362364

363365
@Override
@@ -380,24 +382,26 @@ int valueType() {
380382

381383
@Override
382384
int valueSize() {
385+
int n = computeVarintSize(updateIndexDelta);
383386
switch (valueType()) {
384387
case VALUE_NONE:
385-
return 0;
388+
return n;
386389
case VALUE_1ID:
387-
return OBJECT_ID_LENGTH;
390+
return n + OBJECT_ID_LENGTH;
388391
case VALUE_2ID:
389-
return 2 * OBJECT_ID_LENGTH;
392+
return n + 2 * OBJECT_ID_LENGTH;
390393
case VALUE_SYMREF:
391394
if (ref.isSymbolic()) {
392395
int nameLen = nameUtf8(ref.getTarget()).length;
393-
return computeVarintSize(nameLen) + nameLen;
396+
return n + computeVarintSize(nameLen) + nameLen;
394397
}
395398
}
396399
throw new IllegalStateException();
397400
}
398401

399402
@Override
400403
void writeValue(ReftableOutputStream os) throws IOException {
404+
os.writeVarint(updateIndexDelta);
401405
switch (valueType()) {
402406
case VALUE_NONE:
403407
return;

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/MergedReftable.java

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -100,31 +100,13 @@ public RefCursor allRefs() throws IOException {
100100

101101
@Override
102102
public RefCursor seekRef(String name) throws IOException {
103-
if (name.endsWith("/")) { //$NON-NLS-1$
104-
return seekRefPrefix(name);
105-
}
106-
return seekSingleRef(name);
107-
}
108-
109-
private RefCursor seekRefPrefix(String name) throws IOException {
110103
MergedRefCursor m = new MergedRefCursor();
111104
for (int i = 0; i < tables.length; i++) {
112105
m.add(new RefQueueEntry(tables[i].seekRef(name), i));
113106
}
114107
return m;
115108
}
116109

117-
private RefCursor seekSingleRef(String name) throws IOException {
118-
// Walk the tables from highest priority (end of list) to lowest.
119-
// As soon as the reference is found (queue not empty), all lower
120-
// priority tables are irrelevant as current table shadows them.
121-
MergedRefCursor m = new MergedRefCursor();
122-
for (int i = tables.length - 1; i >= 0 && m.queue.isEmpty(); i--) {
123-
m.add(new RefQueueEntry(tables[i].seekRef(name), i));
124-
}
125-
return m;
126-
}
127-
128110
@Override
129111
public RefCursor byObjectId(AnyObjectId name) throws IOException {
130112
MergedRefCursor m = new MergedRefCursor();
@@ -168,6 +150,7 @@ private class MergedRefCursor extends RefCursor {
168150
private final PriorityQueue<RefQueueEntry> queue;
169151
private RefQueueEntry head;
170152
private Ref ref;
153+
private long updateIndex;
171154

172155
MergedRefCursor() {
173156
queue = new PriorityQueue<>(queueSize(), RefQueueEntry::compare);
@@ -205,6 +188,7 @@ public boolean next() throws IOException {
205188
}
206189

207190
ref = t.rc.getRef();
191+
updateIndex = t.rc.getUpdateIndex();
208192
boolean include = includeDeletes || !t.rc.wasDeleted();
209193
skipShadowedRefs(ref.getName());
210194
add(t);
@@ -239,8 +223,17 @@ public Ref getRef() {
239223
return ref;
240224
}
241225

226+
@Override
227+
public long getUpdateIndex() {
228+
return updateIndex;
229+
}
230+
242231
@Override
243232
public void close() {
233+
if (head != null) {
234+
head.rc.close();
235+
head = null;
236+
}
244237
while (!queue.isEmpty()) {
245238
queue.remove().rc.close();
246239
}
@@ -250,6 +243,10 @@ public void close() {
250243
private static class RefQueueEntry {
251244
static int compare(RefQueueEntry a, RefQueueEntry b) {
252245
int cmp = a.name().compareTo(b.name());
246+
if (cmp == 0) {
247+
// higher updateIndex shadows lower updateIndex.
248+
cmp = Long.signum(b.updateIndex() - a.updateIndex());
249+
}
253250
if (cmp == 0) {
254251
// higher index shadows lower index, so higher index first.
255252
cmp = b.stackIdx - a.stackIdx;
@@ -268,6 +265,10 @@ static int compare(RefQueueEntry a, RefQueueEntry b) {
268265
String name() {
269266
return rc.getRef().getName();
270267
}
268+
269+
long updateIndex() {
270+
return rc.getUpdateIndex();
271+
}
271272
}
272273

273274
private class MergedLogCursor extends LogCursor {

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/RefCursor.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ public abstract class RefCursor implements AutoCloseable {
6161
/** @return reference at the current position. */
6262
public abstract Ref getRef();
6363

64+
/** @return updateIndex that last modified the current reference, */
65+
public abstract long getUpdateIndex();
66+
6467
/** @return {@code true} if the current reference was deleted. */
6568
public boolean wasDeleted() {
6669
Ref r = getRef();

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableCompactor.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ public Stats getStats() {
220220
private void mergeRefs(MergedReftable mr) throws IOException {
221221
try (RefCursor rc = mr.allRefs()) {
222222
while (rc.next()) {
223-
writer.writeRef(rc.getRef());
223+
writer.writeRef(rc.getRef(), rc.getUpdateIndex());
224224
}
225225
}
226226
}

‎org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -455,6 +455,7 @@ private class RefCursorImpl extends RefCursor {
455455
private final boolean prefix;
456456

457457
private Ref ref;
458+
private long updateIndex;
458459
BlockReader block;
459460

460461
RefCursorImpl(long scanEnd, byte[] match, boolean prefix) {
@@ -483,6 +484,7 @@ public boolean next() throws IOException {
483484
return false;
484485
}
485486

487+
updateIndex = minUpdateIndex + block.readUpdateIndexDelta();
486488
ref = block.readRef();
487489
if (!includeDeletes && wasDeleted()) {
488490
continue;
@@ -496,6 +498,11 @@ public Ref getRef() {
496498
return ref;
497499
}
498500

501+
@Override
502+
public long getUpdateIndex() {
503+
return updateIndex;
504+
}
505+
499506
@Override
500507
public void close() {
501508
// Do nothing.
@@ -574,6 +581,7 @@ private class ObjCursorImpl extends RefCursor {
574581
private final ObjectId match;
575582

576583
private Ref ref;
584+
private long updateIndex;
577585
private int listIdx;
578586

579587
private LongList blockPos;
@@ -647,6 +655,7 @@ public boolean next() throws IOException {
647655
}
648656

649657
block.parseKey();
658+
updateIndex = minUpdateIndex + block.readUpdateIndexDelta();
650659
ref = block.readRef();
651660
ObjectId id = ref.getObjectId();
652661
if (id != null && match.equals(id)
@@ -661,6 +670,11 @@ public Ref getRef() {
661670
return ref;
662671
}
663672

673+
@Override
674+
public long getUpdateIndex() {
675+
return updateIndex;
676+
}
677+
664678
@Override
665679
public void close() {
666680
// Do nothing.

0 commit comments

Comments
 (0)