From 47fee2a156433bcb429ea94626df20f931c694f6 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 21 Sep 2016 14:33:09 +0200 Subject: [PATCH 1/5] Add method on Index to look up a single document by key Now that we have Java 8, it is easy to provide a default implementation using a default method. The only thing subinterfaces need to provide is a way to construct a predicate matching exactly one document by key. Index backend authors may also choose to provide a more efficient implementation if necessary. Change-Id: I7d9de2f9c947ced9411b69ed39606dc4ba0b599a --- .../com/google/gerrit/server/index/Index.java | 41 +++++++++++++++++++ .../server/index/account/AccountIndex.java | 7 ++++ .../server/index/change/ChangeIndex.java | 7 ++++ .../query/account/AccountPredicates.java | 2 +- .../query/change/LegacyChangeIdPredicate.java | 2 +- 5 files changed, 57 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/Index.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/Index.java index 533e57c4ed..84eb3bb703 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/Index.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/Index.java @@ -17,8 +17,11 @@ package com.google.gerrit.server.index; import com.google.gerrit.server.query.DataSource; import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.QueryParseException; +import com.google.gwtorm.server.OrmException; import java.io.IOException; +import java.util.List; +import java.util.Optional; /** * Secondary index implementation for arbitrary documents. @@ -90,6 +93,44 @@ public interface Index { DataSource getSource(Predicate p, QueryOptions opts) throws QueryParseException; + /** + * Get a single document from the index. + * + * @param key document key. + * @param opts query options. Options that do not make sense in the context of + * a single document, such as start, will be ignored. + * @return a single document if present. + * @throws IOException + */ + default Optional get(K key, QueryOptions opts) throws IOException { + opts = opts.withStart(0).withLimit(2); + List results; + try { + results = getSource(keyPredicate(key), opts).read().toList(); + } catch (QueryParseException e) { + throw new IOException("Unexpected QueryParseException during get()", e); + } catch (OrmException e) { + throw new IOException(e); + } + switch (results.size()) { + case 0: + return Optional.empty(); + case 1: + return Optional.of(results.get(0)); + default: + throw new IOException("Multiple results found in index for key " + + key + ": " + results); + } + } + + /** + * Get a predicate that looks up a single document by key. + * + * @param key document key. + * @return a single predicate. + */ + Predicate keyPredicate(K key); + /** * Mark whether this index is up-to-date and ready to serve reads. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountIndex.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountIndex.java index cb7b3eff8c..406982ab29 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountIndex.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/account/AccountIndex.java @@ -18,9 +18,16 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.index.Index; import com.google.gerrit.server.index.IndexDefinition; +import com.google.gerrit.server.query.Predicate; +import com.google.gerrit.server.query.account.AccountPredicates; public interface AccountIndex extends Index { public interface Factory extends IndexDefinition.IndexFactory { } + + @Override + default Predicate keyPredicate(Account.Id id) { + return AccountPredicates.id(id); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndex.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndex.java index 9545c0a279..c56880f1a0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndex.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndex.java @@ -17,10 +17,17 @@ package com.google.gerrit.server.index.change; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.index.Index; import com.google.gerrit.server.index.IndexDefinition; +import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.LegacyChangeIdPredicate; public interface ChangeIndex extends Index { public interface Factory extends IndexDefinition.IndexFactory { } + + @Override + default Predicate keyPredicate(Change.Id id) { + return new LegacyChangeIdPredicate(id); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountPredicates.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountPredicates.java index b3f92ff761..9a9ec5d1e4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountPredicates.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountPredicates.java @@ -47,7 +47,7 @@ public class AccountPredicates { return Predicate.or(preds); } - static Predicate id(Account.Id accountId) { + public static Predicate id(Account.Id accountId) { return new AccountPredicate(AccountField.ID, AccountQueryBuilder.FIELD_ACCOUNT, accountId.toString()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LegacyChangeIdPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LegacyChangeIdPredicate.java index 425eb009d2..f7f98d5cdb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LegacyChangeIdPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LegacyChangeIdPredicate.java @@ -22,7 +22,7 @@ import com.google.gerrit.reviewdb.client.Change; public class LegacyChangeIdPredicate extends ChangeIndexPredicate { private final Change.Id id; - LegacyChangeIdPredicate(Change.Id id) { + public LegacyChangeIdPredicate(Change.Id id) { super(LEGACY_ID, ChangeQueryBuilder.FIELD_CHANGE, id.toString()); this.id = id; } From 7d0e67c6ec862ccbee2fc184e4671ab7e2d5ea66 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 22 Nov 2016 08:49:53 -0500 Subject: [PATCH 2/5] Store in index the state of all refs that contribute to a change The primary storage (ReviewDb or NoteDb) and the secondary index (Lucene, etc.) are not updated atomically, so they can get out of sync in various ways. For example, the server may crash before writing to the index, or index updates for concurrent primary storage writes may happen out of order. We want to be able to detect when this has happened. One component of the state of a change is the row version field in the ReviewDb entity; this is already stored in the index in the serialized Change entity. The remainder of a change's state comprises various refs: * NoteDb refs, if NoteDb is the primary storage: * Change meta ref. * Draft comment refs. * Robot comment ref. * Edit refs in the project repo. * Star refs in All-Users. Store each of these in the index in a field REF_STATE, with the simple format "::". In addition to storing the refs that *are* present, we also need to keep track of which refs are *not* present. For example, if only one user is listed in the EDITBY field, then there must exist an edit ref for that user and only that user, and no other users. This is represented by storing a list of ":" values in the REF_STATE_PATTERN field. A change is up to date only if all refs in each matching each are also listed explicitly in REF_STATE. The pattern format is a slightly extended git refspec pattern which allows arbitrarily many '*'s, to deal with the tricky case of "refs/users/UU/UUUU/edit-C/P" => "refs/users/*/edit-C/*". This change implements the REF_STATE and REF_STATE_PATTERN change fields, which require some additional ChangeData methods, as well as structured data types for these fields. These data types live in a new class StalenessChecker, which will be later extended to use them to check for staleness. Change-Id: I622dbbb334b5aca4d58f770ad453e034a5bc9529 --- .../elasticsearch/ElasticChangeIndex.java | 16 ++ .../gerrit/lucene/LuceneChangeIndex.java | 33 ++++ .../google/gerrit/server/index/FieldDef.java | 6 +- .../server/index/change/ChangeField.java | 75 +++++++++ .../index/change/ChangeSchemaDefinitions.java | 6 + .../server/index/change/StalenessChecker.java | 151 +++++++++++++++++ .../gerrit/server/notedb/ChangeNotes.java | 8 +- .../server/notedb/RobotCommentNotes.java | 16 +- .../server/query/change/ChangeData.java | 66 ++++++-- .../index/change/StalenessCheckerTest.java | 154 ++++++++++++++++++ 10 files changed, 509 insertions(+), 22 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index afd5756a8c..7960200f37 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java @@ -23,6 +23,7 @@ import static org.apache.commons.codec.binary.Base64.decodeBase64; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.elasticsearch.ElasticMapping.MappingProperties; @@ -398,6 +399,21 @@ class ElasticChangeIndex extends AbstractElasticIndex ChangeField.STORED_SUBMIT_RECORD_LENIENT.getName(), ChangeField.SUBMIT_RULE_OPTIONS_LENIENT, cd); + if (source.get(ChangeField.REF_STATE.getName()) != null) { + JsonArray refStates = + source.get(ChangeField.REF_STATE.getName()).getAsJsonArray(); + cd.setRefStates( + Iterables.transform( + refStates, e -> Base64.decodeBase64(e.getAsString()))); + } + if (source.get(ChangeField.REF_STATE_PATTERN.getName()) != null) { + JsonArray refStatePatterns = source.get( + ChangeField.REF_STATE_PATTERN.getName()).getAsJsonArray(); + cd.setRefStatePatterns( + Iterables.transform( + refStatePatterns, e -> Base64.decodeBase64(e.getAsString()))); + } + return cd; } diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 13cd10ddae..2cc784729e 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -22,6 +22,8 @@ import static com.google.gerrit.server.index.change.ChangeField.PROJECT; import static com.google.gerrit.server.index.change.ChangeIndexRewriter.CLOSED_STATUSES; import static com.google.gerrit.server.index.change.ChangeIndexRewriter.OPEN_STATUSES; +import static java.util.stream.Collectors.toList; + import com.google.common.base.Throwables; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Collections2; @@ -120,6 +122,9 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String DELETED_FIELD = ChangeField.DELETED.getName(); private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName(); private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName(); + private static final String REF_STATE_FIELD = ChangeField.REF_STATE.getName(); + private static final String REF_STATE_PATTERN_FIELD = + ChangeField.REF_STATE_PATTERN.getName(); private static final String REVIEWEDBY_FIELD = ChangeField.REVIEWEDBY.getName(); private static final String REVIEWER_FIELD = ChangeField.REVIEWER.getName(); @@ -472,6 +477,12 @@ public class LuceneChangeIndex implements ChangeIndex { ChangeField.SUBMIT_RULE_OPTIONS_STRICT, cd); decodeSubmitRecords(doc, SUBMIT_RECORD_LENIENT_FIELD, ChangeField.SUBMIT_RULE_OPTIONS_LENIENT, cd); + if (fields.contains(REF_STATE_FIELD)) { + decodeRefStates(doc, cd); + } + if (fields.contains(REF_STATE_PATTERN_FIELD)) { + decodeRefStatePatterns(doc, cd); + } return cd; } @@ -572,6 +583,16 @@ public class LuceneChangeIndex implements ChangeIndex { opts, cd); } + private void decodeRefStates(Multimap doc, + ChangeData cd) { + cd.setRefStates(copyAsBytes(doc.get(REF_STATE_FIELD))); + } + + private void decodeRefStatePatterns(Multimap doc, + ChangeData cd) { + cd.setRefStatePatterns(copyAsBytes(doc.get(REF_STATE_PATTERN_FIELD))); + } + private static List decodeProtos(Multimap doc, String fieldName, ProtobufCodec codec) { Collection fields = doc.get(fieldName); @@ -586,4 +607,16 @@ public class LuceneChangeIndex implements ChangeIndex { } return result; } + + private static List copyAsBytes(Collection fields) { + return fields.stream() + .map( + f -> { + BytesRef ref = f.binaryValue(); + byte[] b = new byte[ref.length]; + System.arraycopy(ref.bytes, ref.offset, b, 0, ref.length); + return b; + }) + .collect(toList()); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java index 386092df88..92938cba9e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.base.CharMatcher; import com.google.common.base.Preconditions; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.TrackingFooters; import com.google.gwtorm.server.OrmException; @@ -65,14 +66,17 @@ public abstract class FieldDef { public static class FillArgs { public final TrackingFooters trackingFooters; public final boolean allowsDrafts; + public final AllUsersName allUsers; @Inject FillArgs(TrackingFooters trackingFooters, - @GerritServerConfig Config cfg) { + @GerritServerConfig Config cfg, + AllUsersName allUsers) { this.trackingFooters = trackingFooters; this.allowsDrafts = cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true); + this.allUsers = allUsers; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java index 225b756c69..f92e3232b4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java @@ -36,13 +36,20 @@ import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.index.FieldDef; import com.google.gerrit.server.index.FieldType; import com.google.gerrit.server.index.SchemaUtil; +import com.google.gerrit.server.index.change.StalenessChecker.RefState; +import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.ReviewerStateInternal; +import com.google.gerrit.server.notedb.RobotCommentNotes; import com.google.gerrit.server.project.SubmitRuleOptions; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; @@ -945,6 +952,74 @@ public class ChangeField { return result; } + /** + * All values of all refs that were used in the course of indexing this + * document. + *

+ * Emitted as UTF-8 encoded strings of the form + * {@code project:ref/name:[hex sha]}. + */ + public static final FieldDef> REF_STATE = + new FieldDef.Repeatable( + "ref_state", FieldType.STORED_ONLY, true) { + @Override + public Iterable get(ChangeData input, FillArgs args) + throws OrmException { + List result = new ArrayList<>(); + Project.NameKey project = input.change().getProject(); + + input.editRefs().values().forEach( + r -> result.add(RefState.of(r).toByteArray(project))); + input.starRefs().values().forEach( + r -> result.add(RefState.of(r.ref()).toByteArray(project))); + + if (PrimaryStorage.of(input.change()) == PrimaryStorage.NOTE_DB) { + ChangeNotes notes = input.notes(); + result.add(RefState.create(notes.getRefName(), notes.getMetaId()) + .toByteArray(project)); + notes.getRobotComments(); // Force loading robot comments. + RobotCommentNotes robotNotes = notes.getRobotCommentNotes(); + result.add( + RefState.create(robotNotes.getRefName(), robotNotes.getMetaId()) + .toByteArray(project)); + input.draftRefs().values().forEach( + r -> result.add(RefState.of(r).toByteArray(args.allUsers))); + } + + return result; + } }; + + /** + * All ref wildcard patterns that were used in the course of indexing this + * document. + *

+ * Emitted as UTF-8 encoded strings of the form {@code project:ref/name/*}. + * See {@link RefStatePattern} for the pattern format. + */ + public static final FieldDef> + REF_STATE_PATTERN = new FieldDef.Repeatable( + "ref_state_pattern", FieldType.STORED_ONLY, true) { + @Override + public Iterable get(ChangeData input, FillArgs args) + throws OrmException { + Change.Id id = input.getId(); + Project.NameKey project = input.change().getProject(); + List result = new ArrayList<>(3); + result.add(RefStatePattern.create( + RefNames.REFS_USERS + "*/" + RefNames.EDIT_PREFIX + id + "/*") + .toByteArray(project)); + if (PrimaryStorage.of(input.change()) == PrimaryStorage.NOTE_DB) { + result.add( + RefStatePattern.create(RefNames.refsStarredChangesPrefix(id) + "*") + .toByteArray(args.allUsers)); + result.add( + RefStatePattern.create(RefNames.refsDraftCommentsPrefix(id) + "*") + .toByteArray(args.allUsers)); + } + return result; + } + }; + public static final Integer NOT_REVIEWED = -1; private static String getTopic(ChangeData input) throws OrmException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java index 8a793e5e11..49bccf2ae8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java @@ -73,12 +73,18 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions { .add(ChangeField.LABEL2) .build(); + @Deprecated static final Schema V35 = schema(V34, ChangeField.SUBMIT_RECORD, ChangeField.STORED_SUBMIT_RECORD_LENIENT, ChangeField.STORED_SUBMIT_RECORD_STRICT); + static final Schema V36 = + schema(V35, + ChangeField.REF_STATE, + ChangeField.REF_STATE_PATTERN); + public static final String NAME = "changes"; public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java new file mode 100644 index 0000000000..f341970dad --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java @@ -0,0 +1,151 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.index.change; + +import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.base.Preconditions.checkArgument; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.joining; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Splitter; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.HashMultimap; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.SetMultimap; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Project; + +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; + +import java.util.List; +import java.util.regex.Pattern; +import java.util.stream.StreamSupport; + +public class StalenessChecker { + public static SetMultimap parseStates( + Iterable states) { + RefState.check(states != null, null); + SetMultimap result = HashMultimap.create(); + for (byte[] b : states) { + RefState.check(b != null, null); + String s = new String(b, UTF_8); + List parts = Splitter.on(':').splitToList(s); + RefState.check( + parts.size() == 3 + && !parts.get(0).isEmpty() + && !parts.get(1).isEmpty(), + s); + result.put( + new Project.NameKey(parts.get(0)), + RefState.create(parts.get(1), parts.get(2))); + } + return result; + } + + public static ListMultimap parsePatterns( + Iterable patterns) { + RefStatePattern.check(patterns != null, null); + ListMultimap result = + ArrayListMultimap.create(); + for (byte[] b : patterns) { + RefStatePattern.check(b != null, null); + String s = new String(b, UTF_8); + List parts = Splitter.on(':').splitToList(s); + RefStatePattern.check(parts.size() == 2, s); + result.put( + new Project.NameKey(parts.get(0)), + RefStatePattern.create(parts.get(1))); + } + return result; + } + + @AutoValue + public abstract static class RefState { + static RefState create(String ref, String sha) { + return new AutoValue_StalenessChecker_RefState( + ref, ObjectId.fromString(sha)); + } + + static RefState create(String ref, @Nullable ObjectId id) { + return new AutoValue_StalenessChecker_RefState( + ref, firstNonNull(id, ObjectId.zeroId())); + } + + static RefState of(Ref ref) { + return new AutoValue_StalenessChecker_RefState( + ref.getName(), ref.getObjectId()); + } + + byte[] toByteArray(Project.NameKey project) { + byte[] a = (project.toString() + ':' + ref() + ':').getBytes(UTF_8); + byte[] b = new byte[a.length + Constants.OBJECT_ID_STRING_LENGTH]; + System.arraycopy(a, 0, b, 0, a.length); + id().copyTo(b, a.length); + return b; + } + + private static void check(boolean condition, String str) { + checkArgument(condition, "invalid RefState: %s", str); + } + + abstract String ref(); + abstract ObjectId id(); + } + + /** + * Pattern for matching refs. + *

+ * Similar to '*' syntax for native Git refspecs, but slightly more powerful: + * the pattern may contain arbitrarily many asterisks. There must be at least + * one '*' and the first one must immediately follow a '/'. + */ + @AutoValue + public abstract static class RefStatePattern { + static RefStatePattern create(String pattern) { + int star = pattern.indexOf('*'); + check(star > 0 && pattern.charAt(star - 1) == '/', pattern); + String prefix = pattern.substring(0, star); + check(Repository.isValidRefName(pattern.replace('*', 'x')), pattern); + + // Quote everything except the '*'s, which become ".*". + String regex = + StreamSupport.stream(Splitter.on('*').split(pattern).spliterator(), false) + .map(Pattern::quote) + .collect(joining(".*", "^", "$")); + return new AutoValue_StalenessChecker_RefStatePattern( + pattern, prefix, Pattern.compile(regex)); + } + + byte[] toByteArray(Project.NameKey project) { + return (project.toString() + ':' + pattern()).getBytes(UTF_8); + } + + private static void check(boolean condition, String str) { + checkArgument(condition, "invalid RefStatePattern: %s", str); + } + + abstract String pattern(); + abstract String prefix(); + abstract Pattern regex(); + + boolean match(String refName) { + return regex().matcher(refName).find(); + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index d0b0c5484b..a3ea3815d1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -372,6 +372,10 @@ public class ChangeNotes extends AbstractChangeNotes { return change; } + public ObjectId getMetaId() { + return state.metaId(); + } + public ImmutableSortedMap getPatchSets() { if (patchSets == null) { ImmutableSortedMap.Builder b = @@ -510,7 +514,7 @@ public class ChangeNotes extends AbstractChangeNotes { return draftCommentNotes; } - RobotCommentNotes getRobotCommentNotes() { + public RobotCommentNotes getRobotCommentNotes() { return robotCommentNotes; } @@ -532,7 +536,7 @@ public class ChangeNotes extends AbstractChangeNotes { } @Override - protected String getRefName() { + public String getRefName() { return changeMetaRef(getChangeId()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java index f60fd2d2b2..ae280f475b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RobotCommentNotes.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.Multimap; +import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; @@ -43,6 +44,7 @@ public class RobotCommentNotes extends AbstractChangeNotes { private ImmutableListMultimap comments; private RevisionNoteMap revisionNoteMap; + private ObjectId metaId; @AssistedInject RobotCommentNotes( @@ -70,20 +72,26 @@ public class RobotCommentNotes extends AbstractChangeNotes { } @Override - protected String getRefName() { + public String getRefName() { return RefNames.robotCommentsRef(getChangeId()); } + @Nullable + public ObjectId getMetaId() { + return metaId; + } + @Override protected void onLoad(LoadHandle handle) throws IOException, ConfigInvalidException { - ObjectId rev = handle.id(); - if (rev == null) { + metaId = handle.id(); + if (metaId == null) { loadDefaults(); return; } + metaId = metaId.copy(); - RevCommit tipCommit = handle.walk().parseCommit(rev); + RevCommit tipCommit = handle.walk().parseCommit(metaId); ObjectReader reader = handle.walk().getObjectReader(); revisionNoteMap = RevisionNoteMap.parseRobotComments(args.noteUtil, reader, NoteMap.read(reader, tipCommit)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 913cffd115..01b4bc19c8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -89,7 +89,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -348,9 +347,9 @@ public class ChangeData { private SubmitTypeRecord submitTypeRecord; private Boolean mergeable; private Set hashtags; - private Set editsByUser; + private Map editsByUser; private Set reviewedBy; - private Set draftsByUser; + private Map draftsByUser; @Deprecated private Set starredByUser; private ImmutableMultimap stars; @@ -360,6 +359,9 @@ public class ChangeData { private PersonIdent author; private PersonIdent committer; + private ImmutableList refStates; + private ImmutableList refStatePatterns; + @AssistedInject private ChangeData( GitRepositoryManager repoManager, @@ -1098,21 +1100,25 @@ public class ChangeData { } public Set editsByUser() throws OrmException { + return editRefs().keySet(); + } + + public Map editRefs() throws OrmException { if (editsByUser == null) { if (!lazyLoad) { - return Collections.emptySet(); + return Collections.emptyMap(); } Change c = change(); if (c == null) { - return Collections.emptySet(); + return Collections.emptyMap(); } - editsByUser = new HashSet<>(); + editsByUser = new HashMap<>(); Change.Id id = checkNotNull(change.getId()); try (Repository repo = repoManager.openRepository(project())) { - for (String ref - : repo.getRefDatabase().getRefs(RefNames.REFS_USERS).keySet()) { - if (id.equals(Change.Id.fromEditRefPart(ref))) { - editsByUser.add(Account.Id.fromRefPart(ref)); + for (Map.Entry e + : repo.getRefDatabase().getRefs(RefNames.REFS_USERS).entrySet()) { + if (id.equals(Change.Id.fromEditRefPart(e.getKey()))) { + editsByUser.put(Account.Id.fromRefPart(e.getKey()), e.getValue()); } } } catch (IOException e) { @@ -1123,17 +1129,31 @@ public class ChangeData { } public Set draftsByUser() throws OrmException { + return draftRefs().keySet(); + } + + public Map draftRefs() throws OrmException { if (draftsByUser == null) { if (!lazyLoad) { - return Collections.emptySet(); + return Collections.emptyMap(); } Change c = change(); if (c == null) { - return Collections.emptySet(); + return Collections.emptyMap(); } - draftsByUser = new HashSet<>(); - for (Comment sc : commentsUtil.draftByChange(db, notes())) { - draftsByUser.add(sc.author.getId()); + + draftsByUser = new HashMap<>(); + if (notesMigration.readChanges()) { + for (Ref ref : commentsUtil.getDraftRefs(notes.getChangeId())) { + Account.Id account = Account.Id.fromRefSuffix(ref.getName()); + if (account != null) { + draftsByUser.put(account, ref); + } + } + } else { + for (Comment sc : commentsUtil.draftByChange(db, notes())) { + draftsByUser.put(sc.author.getId(), null); + } } } return draftsByUser; @@ -1262,4 +1282,20 @@ public class ChangeData { this.deletions = deletions; } } + + public ImmutableList getRefStates() { + return refStates; + } + + public void setRefStates(Iterable refStates) { + this.refStates = ImmutableList.copyOf(refStates); + } + + public ImmutableList getRefStatePatterns() { + return refStatePatterns; + } + + public void setRefStatePatterns(Iterable refStatePatterns) { + this.refStatePatterns = ImmutableList.copyOf(refStatePatterns); + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java new file mode 100644 index 0000000000..4c86fa1a96 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java @@ -0,0 +1,154 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.index.change; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.toList; + +import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.collect.ListMultimap; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.index.change.StalenessChecker.RefState; +import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern; +import com.google.gerrit.testutil.GerritBaseTests; + +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Test; + +import java.util.stream.Stream; + +public class StalenessCheckerTest extends GerritBaseTests { + private static final String SHA1 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + private static final String SHA2 = "badc0feebadc0feebadc0feebadc0feebadc0fee"; + + private static final Project.NameKey P1 = new Project.NameKey("project1"); + private static final Project.NameKey P2 = new Project.NameKey("project2"); + + @Test + public void parseStates() { + assertInvalidState(null); + assertInvalidState(""); + assertInvalidState("project1:refs/heads/foo"); + assertInvalidState("project1:refs/heads/foo:notasha"); + assertInvalidState("project1:refs/heads/foo:"); + + assertThat( + StalenessChecker.parseStates( + byteArrays( + P1 + ":refs/heads/foo:" + SHA1, + P1 + ":refs/heads/bar:" + SHA2, + P2 + ":refs/heads/baz:" + SHA1))) + .isEqualTo( + ImmutableSetMultimap.of( + P1, RefState.create("refs/heads/foo", SHA1), + P1, RefState.create("refs/heads/bar", SHA2), + P2, RefState.create("refs/heads/baz", SHA1))); + } + + private static void assertInvalidState(String state) { + try { + StalenessChecker.parseStates(byteArrays(state)); + assert_().fail("expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // Expected. + } + } + + @Test + public void refStateToByteArray() { + assertThat( + new String( + RefState.create("refs/heads/foo", ObjectId.fromString(SHA1)) + .toByteArray(P1), + UTF_8)) + .isEqualTo(P1 + ":refs/heads/foo:" + SHA1); + assertThat( + new String( + RefState.create("refs/heads/foo", (ObjectId) null) + .toByteArray(P1), + UTF_8)) + .isEqualTo(P1 + ":refs/heads/foo:" + ObjectId.zeroId().name()); + } + + @Test + public void parsePatterns() { + assertInvalidPattern(null); + assertInvalidPattern(""); + assertInvalidPattern("project:"); + assertInvalidPattern("project:refs/heads/foo"); + assertInvalidPattern("project:refs/he*ds/bar"); + assertInvalidPattern("project:refs/(he)*ds/bar"); + assertInvalidPattern("project:invalidrefname"); + + ListMultimap r = + StalenessChecker.parsePatterns( + byteArrays( + P1 + ":refs/heads/*", + P2 + ":refs/heads/foo/*/bar", + P2 + ":refs/heads/foo/*-baz/*/quux")); + + assertThat(r.keySet()).containsExactly(P1, P2); + RefStatePattern p = r.get(P1).get(0); + assertThat(p.pattern()).isEqualTo("refs/heads/*"); + assertThat(p.prefix()).isEqualTo("refs/heads/"); + assertThat(p.regex().pattern()).isEqualTo("^\\Qrefs/heads/\\E.*\\Q\\E$"); + assertThat(p.match("refs/heads/foo")).isTrue(); + assertThat(p.match("xrefs/heads/foo")).isFalse(); + assertThat(p.match("refs/tags/foo")).isFalse(); + + p = r.get(P2).get(0); + assertThat(p.pattern()).isEqualTo("refs/heads/foo/*/bar"); + assertThat(p.prefix()).isEqualTo("refs/heads/foo/"); + assertThat(p.regex().pattern()) + .isEqualTo("^\\Qrefs/heads/foo/\\E.*\\Q/bar\\E$"); + assertThat(p.match("refs/heads/foo//bar")).isTrue(); + assertThat(p.match("refs/heads/foo/x/bar")).isTrue(); + assertThat(p.match("refs/heads/foo/x/y/bar")).isTrue(); + assertThat(p.match("refs/heads/foo/x/baz")).isFalse(); + + p = r.get(P2).get(1); + assertThat(p.pattern()).isEqualTo("refs/heads/foo/*-baz/*/quux"); + assertThat(p.prefix()).isEqualTo("refs/heads/foo/"); + assertThat(p.regex().pattern()) + .isEqualTo("^\\Qrefs/heads/foo/\\E.*\\Q-baz/\\E.*\\Q/quux\\E$"); + assertThat(p.match("refs/heads/foo/-baz//quux")).isTrue(); + assertThat(p.match("refs/heads/foo/x-baz/x/quux")).isTrue(); + assertThat(p.match("refs/heads/foo/x/y-baz/x/y/quux")).isTrue(); + assertThat(p.match("refs/heads/foo/x-baz/x/y")).isFalse(); + } + + @Test + public void refStatePatternToByteArray() { + assertThat( + new String(RefStatePattern.create("refs/*").toByteArray(P1), UTF_8)) + .isEqualTo(P1 + ":refs/*"); + } + + private static void assertInvalidPattern(String state) { + try { + StalenessChecker.parsePatterns(byteArrays(state)); + assert_().fail("expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // Expected. + } + } + + private static Iterable byteArrays(String... strs) { + return Stream.of(strs).map(s -> s != null ? s.getBytes(UTF_8) : null) + .collect(toList()); + } +} From 38eba5f350ea35086293afbbd53ea2a311a3abef Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 22 Nov 2016 10:05:48 -0500 Subject: [PATCH 3/5] Implement staleness checker for index changes As described in I622dbbb3, we use a combination of the change row version, the ref states, and ref patterns stored in the index to check whether the change document stored in the secondary index is up to date with the primary storage. Change-Id: I8d4a63b705527c5b774373f75dbc28ca9fd10158 --- .../server/index/change/StalenessChecker.java | 159 +++++++++++++- .../index/change/StalenessCheckerTest.java | 205 ++++++++++++++++++ 2 files changed, 363 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java index f341970dad..0c3d89c66d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java @@ -20,25 +20,133 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.joining; import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; +import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.index.IndexConfig; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.io.IOException; import java.util.List; +import java.util.Optional; +import java.util.Set; import java.util.regex.Pattern; import java.util.stream.StreamSupport; +@Singleton public class StalenessChecker { - public static SetMultimap parseStates( + private static final Logger log = + LoggerFactory.getLogger(StalenessChecker.class); + + private final ImmutableSet FIELDS = ImmutableSet.of( + ChangeField.CHANGE.getName(), + ChangeField.REF_STATE.getName(), + ChangeField.REF_STATE_PATTERN.getName()); + + private final ChangeIndexCollection indexes; + private final GitRepositoryManager repoManager; + private final IndexConfig indexConfig; + private final Provider db; + + @Inject + StalenessChecker( + ChangeIndexCollection indexes, + GitRepositoryManager repoManager, + IndexConfig indexConfig, + Provider db) { + this.indexes = indexes; + this.repoManager = repoManager; + this.indexConfig = indexConfig; + this.db = db; + } + + boolean isStale(Change.Id id) throws IOException, OrmException { + ChangeIndex i = indexes.getSearchIndex(); + if (i == null) { + return false; // No index; caller couldn't do anything if it is stale. + } + if (!i.getSchema().hasField(ChangeField.REF_STATE) + || !i.getSchema().hasField(ChangeField.REF_STATE_PATTERN)) { + return false; // Index version not new enough for this check. + } + + Optional result = i.get( + id, IndexedChangeQuery.createOptions(indexConfig, 0, 1, FIELDS)); + if (!result.isPresent()) { + return true; // Not in index, but caller wants it to be. + } + ChangeData cd = result.get(); + if (reviewDbChangeIsStale( + cd.change(), + ChangeNotes.readOneReviewDbChange(db.get(), cd.getId()))) { + return true; + } + + return isStale(repoManager, id, parseStates(cd), parsePatterns(cd)); + } + + @VisibleForTesting + static boolean isStale(GitRepositoryManager repoManager, + Change.Id id, + SetMultimap states, + Multimap patterns) { + Set projects = + Sets.union(states.keySet(), patterns.keySet()); + + for (Project.NameKey p : projects) { + if (isStale(repoManager, id, p, states, patterns)) { + return true; + } + } + + return false; + } + + @VisibleForTesting + static boolean reviewDbChangeIsStale( + Change indexChange, @Nullable Change reviewDbChange) { + if (reviewDbChange == null) { + return false; // Nothing the caller can do. + } + checkArgument(indexChange.getId().equals(reviewDbChange.getId()), + "mismatched change ID: %s != %s", + indexChange.getId(), reviewDbChange.getId()); + if (PrimaryStorage.of(reviewDbChange) != PrimaryStorage.REVIEW_DB) { + return false; // Not a ReviewDb change, don't check rowVersion. + } + return reviewDbChange.getRowVersion() != indexChange.getRowVersion(); + } + + private SetMultimap parseStates(ChangeData cd) { + return parseStates(cd.getRefStates()); + } + + @VisibleForTesting + static SetMultimap parseStates( Iterable states) { RefState.check(states != null, null); SetMultimap result = HashMultimap.create(); @@ -58,6 +166,11 @@ public class StalenessChecker { return result; } + private Multimap parsePatterns( + ChangeData cd) { + return parsePatterns(cd.getRefStatePatterns()); + } + public static ListMultimap parsePatterns( Iterable patterns) { RefStatePattern.check(patterns != null, null); @@ -75,6 +188,31 @@ public class StalenessChecker { return result; } + private static boolean isStale(GitRepositoryManager repoManager, + Change.Id id, Project.NameKey project, + SetMultimap allStates, + Multimap allPatterns) { + try (Repository repo = repoManager.openRepository(project)) { + Set states = allStates.get(project); + for (RefState state : states) { + if (!state.match(repo)) { + return true; + } + } + for (RefStatePattern pattern : allPatterns.get(project)) { + if (!pattern.match(repo, states)) { + return true; + } + } + return false; + } catch (IOException e) { + log.warn( + String.format("error checking staleness of %s in %s", id, project), + e); + return true; + } + } + @AutoValue public abstract static class RefState { static RefState create(String ref, String sha) { @@ -106,6 +244,12 @@ public class StalenessChecker { abstract String ref(); abstract ObjectId id(); + + private boolean match(Repository repo) throws IOException { + Ref ref = repo.exactRef(ref()); + ObjectId expected = ref != null ? ref.getObjectId() : ObjectId.zeroId(); + return id().equals(expected); + } } /** @@ -147,5 +291,18 @@ public class StalenessChecker { boolean match(String refName) { return regex().matcher(refName).find(); } + + private boolean match(Repository repo, Set expected) + throws IOException { + for (Ref r : repo.getRefDatabase().getRefs(prefix()).values()) { + if (!match(r.getName())) { + continue; + } + if (!expected.contains(RefState.of(r))) { + return false; + } + } + return true; + } } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java index 4c86fa1a96..cdd1e07891 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java @@ -16,17 +16,30 @@ package com.google.gerrit.server.index.change; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assert_; +import static com.google.gerrit.server.index.change.StalenessChecker.isStale; +import static com.google.gerrit.testutil.TestChanges.newChange; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toList; +import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.ListMultimap; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.StalenessChecker.RefState; import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern; +import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.testutil.GerritBaseTests; +import com.google.gerrit.testutil.InMemoryRepositoryManager; +import com.google.gwtorm.protobuf.CodecFactory; +import com.google.gwtorm.protobuf.ProtobufCodec; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.junit.Before; import org.junit.Test; import java.util.stream.Stream; @@ -38,6 +51,26 @@ public class StalenessCheckerTest extends GerritBaseTests { private static final Project.NameKey P1 = new Project.NameKey("project1"); private static final Project.NameKey P2 = new Project.NameKey("project2"); + private static final Change.Id C = new Change.Id(1234); + + private static final ProtobufCodec CHANGE_CODEC = + CodecFactory.encoder(Change.class); + + private GitRepositoryManager repoManager; + private Repository r1; + private Repository r2; + private TestRepository tr1; + private TestRepository tr2; + + @Before + public void setUp() throws Exception { + repoManager = new InMemoryRepositoryManager(); + r1 = repoManager.createRepository(P1); + tr1 = new TestRepository<>(r1); + r2 = repoManager.createRepository(P2); + tr2 = new TestRepository<>(r2); + } + @Test public void parseStates() { assertInvalidState(null); @@ -147,8 +180,180 @@ public class StalenessCheckerTest extends GerritBaseTests { } } + @Test + public void isStaleRefStatesOnly() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + String ref2 = "refs/heads/bar"; + ObjectId id2 = tr2.update(ref2, tr2.commit().message("commit 2")); + + // Not stale. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P2, RefState.create(ref2, id2.name())), + ImmutableMultimap.of())) + .isFalse(); + + // Wrong ref value. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, SHA1), + P2, RefState.create(ref2, id2.name())), + ImmutableMultimap.of())) + .isTrue(); + + // Swapped repos. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id2.name()), + P2, RefState.create(ref2, id1.name())), + ImmutableMultimap.of())) + .isTrue(); + + // Two refs in same repo, not stale. + String ref3 = "refs/heads/baz"; + ObjectId id3 = tr1.update(ref3, tr1.commit().message("commit 3")); + tr1.update(ref3, id3); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, id3.name())), + ImmutableMultimap.of())) + .isFalse(); + + // Ignore ref not mentioned. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of())) + .isFalse(); + + // One ref wrong. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, SHA1)), + ImmutableMultimap.of())) + .isTrue(); + } + + @Test + public void isStaleWithRefStatePatterns() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + + // ref1 is only ref matching pattern. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isFalse(); + + // Now ref2 matches pattern, so stale unless ref2 is present in state map. + String ref2 = "refs/heads/bar"; + ObjectId id2 = tr1.update(ref2, tr1.commit().message("commit 2")); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isTrue(); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref2, id2.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isFalse(); + } + + @Test + public void isStaleWithNonPrefixPattern() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + tr1.update("refs/heads/bar", tr1.commit().message("commit 2")); + + // ref1 is only ref matching pattern. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isFalse(); + + // Now ref2 matches pattern, so stale unless ref2 is present in state map. + String ref3 = "refs/other/foo"; + ObjectId id3 = tr1.update(ref3, tr1.commit().message("commit 3")); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isTrue(); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, id3.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isFalse(); + } + + @Test + public void reviewDbChangeIsStale() throws Exception { + Change indexChange = newChange(P1, new Account.Id(1)); + indexChange.setNoteDbState(SHA1); + + assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, null)) + .isFalse(); + + Change noteDbPrimary = clone(indexChange); + noteDbPrimary.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); + assertThat( + StalenessChecker.reviewDbChangeIsStale(indexChange, noteDbPrimary)) + .isFalse(); + + assertThat( + StalenessChecker.reviewDbChangeIsStale( + indexChange, clone(indexChange))) + .isFalse(); + + // Can't easily change row version to check true case. + } + private static Iterable byteArrays(String... strs) { return Stream.of(strs).map(s -> s != null ? s.getBytes(UTF_8) : null) .collect(toList()); } + + private static Change clone(Change change) { + return CHANGE_CODEC.decode(CHANGE_CODEC.encodeToByteArray(change)); + } + } From 4eb8d92adbe6a6b908f70905a5641521792def1a Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 22 Nov 2016 09:13:34 -0500 Subject: [PATCH 4/5] ChangeIndexer: Add method to reindex a change if it is stale At this point we have enough wiring to write a larger test in AbstractQueryChangesTest for StalenessChecker. Change-Id: Ia17351761fb4ac3ed56e360f28fd2f0a9e3c3ed0 --- .../server/index/change/ChangeIndexer.java | 93 ++++++++++++++++--- .../change/AbstractQueryChangesTest.java | 27 ++++++ 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java index f881f2eed4..507edfbf7c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.index.change; +import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH; import static com.google.gerrit.server.extensions.events.EventUtil.logEventListenerError; import com.google.common.base.Function; @@ -29,6 +30,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.index.Index; +import com.google.gerrit.server.index.IndexExecutor; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.query.change.ChangeData; @@ -102,8 +104,10 @@ public class ChangeIndexer { private final ChangeNotes.Factory changeNotesFactory; private final ChangeData.Factory changeDataFactory; private final ThreadLocalRequestContext context; + private final ListeningExecutorService batchExecutor; private final ListeningExecutorService executor; private final DynamicSet indexedListeners; + private final StalenessChecker stalenessChecker; @AssistedInject ChangeIndexer(SchemaFactory schemaFactory, @@ -112,6 +116,8 @@ public class ChangeIndexer { ChangeData.Factory changeDataFactory, ThreadLocalRequestContext context, DynamicSet indexedListeners, + StalenessChecker stalenessChecker, + @IndexExecutor(BATCH) ListeningExecutorService batchExecutor, @Assisted ListeningExecutorService executor, @Assisted ChangeIndex index) { this.executor = executor; @@ -121,6 +127,8 @@ public class ChangeIndexer { this.changeDataFactory = changeDataFactory; this.context = context; this.indexedListeners = indexedListeners; + this.stalenessChecker = stalenessChecker; + this.batchExecutor = batchExecutor; this.index = index; this.indexes = null; } @@ -132,6 +140,8 @@ public class ChangeIndexer { ChangeData.Factory changeDataFactory, ThreadLocalRequestContext context, DynamicSet indexedListeners, + StalenessChecker stalenessChecker, + @IndexExecutor(BATCH) ListeningExecutorService batchExecutor, @Assisted ListeningExecutorService executor, @Assisted ChangeIndexCollection indexes) { this.executor = executor; @@ -141,6 +151,8 @@ public class ChangeIndexer { this.changeDataFactory = changeDataFactory; this.context = context; this.indexedListeners = indexedListeners; + this.stalenessChecker = stalenessChecker; + this.batchExecutor = batchExecutor; this.index = null; this.indexes = indexes; } @@ -245,28 +257,54 @@ public class ChangeIndexer { new DeleteTask(id).call(); } + /** + * Asynchronously check if a change is stale, and reindex if it is. + *

+ * Always run on the batch executor, even if this indexer instance is + * configured to use a different executor. + * + * @param project the project to which the change belongs. + * @param id ID of the change to index. + * @return future for reindexing the change; returns true if the change was + * stale. + */ + public CheckedFuture reindexIfStale( + Project.NameKey project, Change.Id id) { + return submit(new ReindexIfStaleTask(project, id), batchExecutor); + } + private Collection getWriteIndexes() { return indexes != null ? indexes.getWriteIndexes() : Collections.singleton(index); } - private CheckedFuture submit(Callable task) { + private CheckedFuture submit(Callable task) { + return submit(task, executor); + } + + private static CheckedFuture submit(Callable task, + ListeningExecutorService executor) { return Futures.makeChecked( Futures.nonCancellationPropagating(executor.submit(task)), MAPPER); } - private class IndexTask implements Callable { - private final Project.NameKey project; - private final Change.Id id; + private abstract class AbstractIndexTask implements Callable { + protected final Project.NameKey project; + protected final Change.Id id; - private IndexTask(Project.NameKey project, Change.Id id) { + protected AbstractIndexTask(Project.NameKey project, Change.Id id) { this.project = project; this.id = id; } + protected abstract T callImpl(Provider db) throws Exception; + @Override - public Void call() throws Exception { + public abstract String toString(); + + @Override + public final T call() throws Exception { try { final AtomicReference> dbRef = Atomics.newReference(); @@ -295,10 +333,7 @@ public class ChangeIndexer { }; RequestContext oldCtx = context.setContext(newCtx); try { - ChangeData cd = newChangeData( - newCtx.getReviewDbProvider().get(), project, id); - index(cd); - return null; + return callImpl(newCtx.getReviewDbProvider()); } finally { context.setContext(oldCtx); Provider db = dbRef.get(); @@ -307,17 +342,31 @@ public class ChangeIndexer { } } } catch (Exception e) { - log.error(String.format("Failed to index change %d", id.get()), e); + log.error("Failed to execute " + this, e); throw e; } } + } + + private class IndexTask extends AbstractIndexTask { + private IndexTask(Project.NameKey project, Change.Id id) { + super(project, id); + } + + @Override + public Void callImpl(Provider db) throws Exception { + ChangeData cd = newChangeData(db.get(), project, id); + index(cd); + return null; + } @Override public String toString() { - return "index-change-" + id.get(); + return "index-change-" + id; } } + // Not AbstractIndexTask as it doesn't need ReviewDb. private class DeleteTask implements Callable { private final Change.Id id; @@ -339,6 +388,26 @@ public class ChangeIndexer { } } + private class ReindexIfStaleTask extends AbstractIndexTask { + private ReindexIfStaleTask(Project.NameKey project, Change.Id id) { + super(project, id); + } + + @Override + public Boolean callImpl(Provider db) throws Exception { + if (!stalenessChecker.isStale(id)) { + return false; + } + index(newChangeData(db.get(), project, id)); + return true; + } + + @Override + public String toString() { + return "reindex-if-stale-change-" + id; + } + } + // Avoid auto-rebuilding when reindexing if reading is disabled. This just // increases contention on the meta ref from a background indexing thread // with little benefit. The next actual write to the entity may still incur a diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 5899e115f6..d22e7a85eb 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -52,6 +52,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; @@ -1611,6 +1612,32 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { cd.currentApprovals(); } + @Test + public void reindexIfStale() throws Exception { + Account.Id user = createAccount("user"); + Project.NameKey project = new Project.NameKey("repo"); + TestRepository repo = createProject(project.get()); + Change change = insert(repo, newChange(repo)); + PatchSet ps = db.patchSets().get(change.currentPatchSetId()); + + requestContext.setContext(newRequestContext(user)); + assertThat(changeEditModifier.createEdit(change, ps)) + .isEqualTo(RefUpdate.Result.NEW); + assertQuery("has:edit", change); + assertThat(indexer.reindexIfStale(project, change.getId()).get()).isFalse(); + + // Delete edit ref behind index's back. + RefUpdate ru = repo.getRepository().updateRef( + RefNames.refsEdit(user, change.getId(), ps.getId())); + ru.setForceUpdate(true); + assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); + + // Index is stale. + assertQuery("has:edit", change); + assertThat(indexer.reindexIfStale(project, change.getId()).get()).isTrue(); + assertQuery("has:edit"); + } + protected ChangeInserter newChange(TestRepository repo) throws Exception { return newChange(repo, null, null, null, null); From 27ec2bad32b4cc81e3a7d0bdf2b502ab5930fa6b Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 22 Nov 2016 12:01:12 -0500 Subject: [PATCH 5/5] ChangeIndexer: Reindex if stale after every index update The index system is subject to a race condition when index updates happen out of order with respect to their primary storage updates. Previously, we had no way of detecting this case at all, and we potentially stored stale data in the index indefinitely. This particular issue was probably not very serious in practice (not that we have any data on it), because if a change is busy enough to hit this race condition, probably there is at least one non-racy write somewhere in its future, so we would have indexed the right data at some point. Change-Id: I04ac36ea55af2cb1e1126365cc6b011e0fb8548c --- .../server/change/GetRelatedIT.java | 2 + .../server/notedb/ChangeRebuilderIT.java | 7 +++ .../server/index/change/ChangeIndexer.java | 57 ++++++++++++++++++- 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java index 37ced5fe73..a39f30082f 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java @@ -21,6 +21,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.common.RawInputUtil; @@ -648,6 +649,7 @@ public class GetRelatedIT extends AbstractDaemonTest { } @Test + @GerritConfig(name = "index.testReindexAfterUpdate", value = "false") public void getRelatedForStaleChange() throws Exception { RevCommit c1_1 = commitBuilder() .add("a.txt", "1") diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index a2b93cc08d..7ab1753e1c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java @@ -110,6 +110,13 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { public static Config defaultConfig() { Config cfg = new Config(); cfg.setBoolean("noteDb", null, "testRebuilderWrapper", true); + + // Disable async reindex-if-stale check after index update. This avoids + // unintentional auto-rebuilding of the change in NoteDb during the read + // path of the reindex-if-stale check. For the purposes of this test, we + // want precise control over when auto-rebuilding happens. + cfg.setBoolean("index", null, "testReindexAfterUpdate", false); + return cfg; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java index 507edfbf7c..f2567070d9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -14,8 +14,8 @@ package com.google.gerrit.server.index.change; -import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH; import static com.google.gerrit.server.extensions.events.EventUtil.logEventListenerError; +import static com.google.gerrit.server.git.QueueProvider.QueueType.BATCH; import com.google.common.base.Function; import com.google.common.util.concurrent.Atomics; @@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.index.Index; import com.google.gerrit.server.index.IndexExecutor; import com.google.gerrit.server.notedb.ChangeNotes; @@ -45,6 +46,7 @@ import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; import com.google.inject.util.Providers; +import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -108,9 +110,12 @@ public class ChangeIndexer { private final ListeningExecutorService executor; private final DynamicSet indexedListeners; private final StalenessChecker stalenessChecker; + private final boolean reindexAfterIndexUpdate; @AssistedInject - ChangeIndexer(SchemaFactory schemaFactory, + ChangeIndexer( + @GerritServerConfig Config cfg, + SchemaFactory schemaFactory, NotesMigration notesMigration, ChangeNotes.Factory changeNotesFactory, ChangeData.Factory changeDataFactory, @@ -129,12 +134,14 @@ public class ChangeIndexer { this.indexedListeners = indexedListeners; this.stalenessChecker = stalenessChecker; this.batchExecutor = batchExecutor; + this.reindexAfterIndexUpdate = reindexAfterIndexUpdate(cfg); this.index = index; this.indexes = null; } @AssistedInject ChangeIndexer(SchemaFactory schemaFactory, + @GerritServerConfig Config cfg, NotesMigration notesMigration, ChangeNotes.Factory changeNotesFactory, ChangeData.Factory changeDataFactory, @@ -153,10 +160,15 @@ public class ChangeIndexer { this.indexedListeners = indexedListeners; this.stalenessChecker = stalenessChecker; this.batchExecutor = batchExecutor; + this.reindexAfterIndexUpdate = reindexAfterIndexUpdate(cfg); this.index = null; this.indexes = indexes; } + private static boolean reindexAfterIndexUpdate(Config cfg) { + return cfg.getBoolean("index", null, "testReindexAfterUpdate", true); + } + /** * Start indexing a change. * @@ -193,6 +205,26 @@ public class ChangeIndexer { i.replace(cd); } fireChangeIndexedEvent(cd.getId().get()); + + // Always double-check whether the change might be stale immediately after + // interactively indexing it. This fixes up the case where two writers write + // to the primary storage in one order, and the corresponding index writes + // happen in the opposite order: + // 1. Writer A writes to primary storage. + // 2. Writer B writes to primary storage. + // 3. Writer B updates index. + // 4. Writer A updates index. + // + // Without the extra reindexIfStale step, A has no way of knowing that it's + // about to overwrite the index document with stale data. It doesn't work to + // have A check for staleness before attempting its index update, because + // B's index update might not have happened when it does the check. + // + // With the extra reindexIfStale step after (3)/(4), we are able to detect + // and fix the staleness. It doesn't matter which order the two + // reindexIfStale calls actually execute in; we are guaranteed that at least + // one of them will execute after the second index write, (4). + reindexAfterIndexUpdate(cd); } private void fireChangeIndexedEvent(int id) { @@ -224,6 +256,8 @@ public class ChangeIndexer { public void index(ReviewDb db, Change change) throws IOException, OrmException { index(newChangeData(db, change)); + // See comment in #index(ChangeData). + reindexAfterIndexUpdate(change.getProject(), change.getId()); } /** @@ -235,7 +269,10 @@ public class ChangeIndexer { */ public void index(ReviewDb db, Project.NameKey project, Change.Id changeId) throws IOException, OrmException { - index(newChangeData(db, project, changeId)); + ChangeData cd = newChangeData(db, project, changeId); + index(cd); + // See comment in #index(ChangeData). + reindexAfterIndexUpdate(cd); } /** @@ -273,6 +310,20 @@ public class ChangeIndexer { return submit(new ReindexIfStaleTask(project, id), batchExecutor); } + private void reindexAfterIndexUpdate(ChangeData cd) throws IOException { + try { + reindexAfterIndexUpdate(cd.project(), cd.getId()); + } catch (OrmException e) { + throw new IOException(e); + } + } + + private void reindexAfterIndexUpdate(Project.NameKey project, Change.Id id) { + if (reindexAfterIndexUpdate) { + reindexIfStale(project, id); + } + } + private Collection getWriteIndexes() { return indexes != null ? indexes.getWriteIndexes()