Fix event plugin possible transaction reuse bug
With the previous design, it was possible for a proposal for an old
transaction to be replayed after the transaction had been completed and
deleted by another actor. This can result an outdated sequence number
from being reused and losing an old event. Previously proposals were
renamed to update/<uuid>/ which could happen at the wrong time after the
<uuid> dir was deleted. Fix this by introducing the 'next' directory. In
the new design, all actors must rename their next proposals to
<uuid>/next. This prevents outdated proposals for old transactions from
being used after the <uuid> dir is deleted.
Testing FsStoreTest with -j 100 and a count of 1000 shows that the
previous design would result in many reused ids, around 150 out of 100K
events. With the same settings, there were no errors with the new
design.
Testing also show this to be compatible with the previous design. While
the previous and new designs cannot complete each other's transactions,
they do not interfere with each other. This should make a multi master
transition while both the old and new designs are running acceptable.
The new design is slower in the single threaded case due to the extra
directory being created and deleted. The new design appears to be faster
50% faster with 100 threads, possibly due to the bug fix preventing more
collisions, although it is hard to make an apples to apples comparison
due to the buggy nature of the previous design.
Change-Id: I4bbea6c8a17b84ac84cf7906825ad65f9f7f5301
diff --git a/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/EventSequence.java b/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/EventSequence.java
index cc26ad2..9ab49fd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/EventSequence.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/EventSequence.java
@@ -37,8 +37,8 @@
protected static class EventBuilder extends UpdatableFileValue.UpdateBuilder {
public EventBuilder(BasePaths paths, String event) throws IOException {
super(paths);
- FileValue.prepare(udir.resolve(EVENT), event); // Phase 1+
- // build/<tmp>/<uuid>/event
+ FileValue.prepare(next.resolve(EVENT), event); // Phase 1+
+ // build/<tmp>/<uuid>/next/event
}
}
@@ -49,7 +49,7 @@
/** Advance through phases 2 - 6 */
protected UniqueUpdate(String uuid, boolean ours, long maxTries) throws IOException {
super(EventSequence.this, uuid, ours, maxTries);
- event = upaths.udir.resolve(EVENT);
+ event = upaths.next.resolve(EVENT);
spinFinish();
}
@@ -63,8 +63,8 @@
protected void storeEvent() throws IOException {
Path destination = getEventDestination(next);
// Phase 2+
- if (Fs.tryAtomicMove(event, destination)) { // rename update/<uuid>/event -> destination
- // now there should be: update/<uuid>/ and destination (file)
+ if (Fs.tryAtomicMove(event, destination)) { // rename update/<uuid>/next/event -> destination
+ // now there should be: update/<uuid>/next/ and destination (file)
this.destination = destination;
}
}
@@ -85,9 +85,9 @@
protected UniqueUpdate spinSubmit(String event, long maxTries) throws IOException {
try (EventBuilder b = new EventBuilder(paths, event)) {
for (long tries = 0; tries < maxTries; tries++) {
- // Phase 1
+ // Phase 1 (can only succeed if update is empty or non-existant)
if (Fs.tryAtomicMove(b.dir, paths.update)) { // rename build/<tmp>/ -> update/
- // now there should be: update/<uuid>/event
+ // now there should be: update/<uuid>/next/event
synchronized (this) {
totalUpdates++;
totalSpins += tries - 1;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/UpdatableFileValue.java b/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/UpdatableFileValue.java
index 6f41b43..59cb13e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/UpdatableFileValue.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/events/fsstore/UpdatableFileValue.java
@@ -46,6 +46,7 @@
public abstract class UpdatableFileValue<T> extends FileValue<T> {
public static final Path CLOSED = Paths.get("closed");
public static final Path INIT = Paths.get("init");
+ public static final Path NEXT = Paths.get("next");
public static final Path PRESERVED = Paths.get("preserved");
public static final Path UPDATE = Paths.get("update");
public static final Path VALUE = Paths.get("value");
@@ -65,10 +66,12 @@
protected static class UpdateBuilder extends FsTransaction.Builder {
public final String uuid = UUID.randomUUID().toString();
public final Path udir = dir.resolve(uuid);
+ public final Path next;
public UpdateBuilder(BasePaths paths) throws IOException {
super(paths);
- Files.createDirectories(udir); // build/<tmp>/<uuid>/
+ next = udir.resolve(NEXT);
+ Files.createDirectories(next); // build/<tmp>/<uuid>/next
}
}
@@ -92,12 +95,14 @@
protected static class UpdatePaths {
public final Path udir;
+ public final Path next;
public final Path closed;
public final Path value;
protected UpdatePaths(Path base, String uuid) {
udir = base.resolve(uuid);
- closed = udir.resolve(CLOSED);
+ next = udir.resolve(NEXT);
+ closed = next.resolve(CLOSED);
value = closed.resolve(VALUE);
}
}
@@ -181,16 +186,18 @@
if (!closed) {
try (NextBuilder b =
new NextBuilder(updatable.paths, updatable.serializer.fromGeneric(next))) { // Phase 3
- // Phase 4
- Fs.tryAtomicMove(b.dir, upaths.udir); // rename build/<tmp>/ -> update/<uuid>/
- // now there should be: update/<uuid>/closed/value(next)
+
+ // Phase 4. Rename can only succeed if update/<uuid>/next/ is empty (desired) or
+ // non-existent (not desired). The later case is detected after the move.
+ Fs.tryAtomicMove(b.dir, upaths.next); // rename build/<tmp>/ -> update/<uuid>/next/
+ // now there should be: update/<uuid>/next/closed/value(next)
}
// Do not use the result of the move to determine if it is closed.
// The move result could provide false positives due to some filesystem
// implementions allowing a second move to succeed after the transaction
// has been finished and the first "closed" has been deleted under the
- // "delete" dir). Additionally, this check allows us to be able to detect
+ // "delete" dir. Additionally, this check allows us to be able to detect
// closes by other actors, not just ourselves.
closed = Files.exists(upaths.closed);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/events/fsstore/FsStoreTest.java b/src/test/java/com/googlesource/gerrit/plugins/events/fsstore/FsStoreTest.java
index 76ccbbb..fb7539d 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/events/fsstore/FsStoreTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/events/fsstore/FsStoreTest.java
@@ -247,14 +247,7 @@
*
* <p>Note: if you do not specify <dir>, it will create a directory under /tmp
*
- * <p>Performance: NFS(Lowlatency,SSDs), 1 worker 1M, 266m ~16ms/event find events|wc -l 12s rm
- * -rf 1m49s du -sh -> 3.9G 1m7s
- *
- * <p>Local(spinning) 1 workers 1M 21m58s ~13ms/event find events|wc -l 1.3s rm -rf 34s
- *
- * <p>Multi workers: NFS(LowLatency,LAN,SSDs) 8 hosts count=1000 (each) avg 273s 1000/34s
- *
- * <p>Mixed workers: NFS(WAN) 1 worker (+NFS LAN continuous) count=10, 3m28s
+ * <p>Local(spinning) 1 worker 1M 28m2s ~17ms/event find events|wc -l .97 rm -rf 30s
*/
public static void main(String[] argv) throws Exception {
List<String> args = new LinkedList<>();
