diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index dd5ced6683..84770c9ef4 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3396,6 +3396,18 @@ contention (aka lock failure) on the underlying ref storage. Defaults to 20 seconds; unit suffixes are supported, and assumes milliseconds if not specified. +[[noteDb.groups.readSequenceFromNoteDb]]noteDb.groups.readSequenceFromNoteDb:: ++ +Whether the group sequence should be read from NoteDb. ++ +Once set to `true` this parameter cannot be set back to `false` because +the group sequence in ReviewDb will no longer be updated when group +IDs are retrieved from NoteDb, and hence the group sequence in +ReviewDb will be outdated. ++ +By default `false`. + + [[oauth]] === Section oauth diff --git a/java/com/google/gerrit/reviewdb/server/ReviewDb.java b/java/com/google/gerrit/reviewdb/server/ReviewDb.java index 04567bc0a8..739d2e66fd 100644 --- a/java/com/google/gerrit/reviewdb/server/ReviewDb.java +++ b/java/com/google/gerrit/reviewdb/server/ReviewDb.java @@ -109,8 +109,11 @@ public interface ReviewDb extends Schema { @Deprecated int nextAccountId() throws OrmException; + int FIRST_GROUP_ID = 1; + /** Next unique id for a {@link AccountGroup}. */ - @Sequence + @Sequence(startWith = FIRST_GROUP_ID) + @Deprecated int nextAccountGroupId() throws OrmException; int FIRST_CHANGE_ID = 1; diff --git a/java/com/google/gerrit/server/Sequences.java b/java/com/google/gerrit/server/Sequences.java index 930f3f3e32..bbbbe496e8 100644 --- a/java/com/google/gerrit/server/Sequences.java +++ b/java/com/google/gerrit/server/Sequences.java @@ -42,6 +42,7 @@ import org.eclipse.jgit.lib.Config; @Singleton public class Sequences { public static final String NAME_ACCOUNTS = "accounts"; + public static final String NAME_GROUPS = "groups"; public static final String NAME_CHANGES = "changes"; public static int getChangeSequenceGap(Config cfg) { @@ -50,17 +51,20 @@ public class Sequences { private enum SequenceType { ACCOUNTS, - CHANGES; + CHANGES, + GROUPS; } private final Provider db; private final NotesMigration migration; private final RepoSequence accountSeq; private final RepoSequence changeSeq; + private final RepoSequence groupSeq; + private final boolean readGroupSeqFromNoteDb; private final Timer2 nextIdLatency; @Inject - Sequences( + public Sequences( @GerritServerConfig Config cfg, Provider db, NotesMigration migration, @@ -90,6 +94,13 @@ public class Sequences { new RepoSequence( repoManager, gitRefUpdated, allProjects, NAME_CHANGES, changeSeed, changeBatchSize); + RepoSequence.Seed groupSeed = () -> nextGroupId(db.get()); + int groupBatchSize = 1; + groupSeq = + new RepoSequence( + repoManager, gitRefUpdated, allUsers, NAME_GROUPS, groupSeed, groupBatchSize); + readGroupSeqFromNoteDb = readGroupFromNoteDbSetting(cfg); + nextIdLatency = metrics.newTimer( "sequence/next_id_latency", @@ -100,6 +111,10 @@ public class Sequences { Field.ofBoolean("multiple")); } + public static boolean readGroupFromNoteDbSetting(Config cfg) { + return cfg.getBoolean("noteDb", "groups", "readSequenceFromNoteDb", false); + } + public int nextAccountId() throws OrmException { try (Timer2.Context timer = nextIdLatency.start(SequenceType.ACCOUNTS, false)) { return accountSeq.next(); @@ -134,6 +149,17 @@ public class Sequences { return ImmutableList.copyOf(ids); } + public int nextGroupId() throws OrmException { + if (readGroupSeqFromNoteDb) { + try (Timer2.Context timer = nextIdLatency.start(SequenceType.GROUPS, false)) { + return groupSeq.next(); + } + } + int groupId = nextGroupId(db.get()); + groupSeq.increaseTo(groupId + 1); // NoteDb stores next available group ID. + return groupId; + } + @VisibleForTesting public RepoSequence getChangeIdRepoSequence() { return changeSeq; @@ -143,4 +169,9 @@ public class Sequences { private static int nextChangeId(ReviewDb db) throws OrmException { return db.nextChangeId(); } + + @SuppressWarnings("deprecation") + static int nextGroupId(ReviewDb db) throws OrmException { + return db.nextAccountGroupId(); + } } diff --git a/java/com/google/gerrit/server/group/CreateGroup.java b/java/com/google/gerrit/server/group/CreateGroup.java index 585cbf54a0..33e32b2921 100644 --- a/java/com/google/gerrit/server/group/CreateGroup.java +++ b/java/com/google/gerrit/server/group/CreateGroup.java @@ -39,6 +39,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.Sequences; import com.google.gerrit.server.account.CreateGroupArgs; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.account.GroupUUID; @@ -82,6 +83,7 @@ public class CreateGroup implements RestModifyView private final SystemGroupBackend systemGroupBackend; private final boolean defaultVisibleToAll; private final String name; + private final Sequences sequences; @Inject CreateGroup( @@ -96,7 +98,8 @@ public class CreateGroup implements RestModifyView AddMembers addMembers, SystemGroupBackend systemGroupBackend, @GerritServerConfig Config cfg, - @Assisted String name) { + @Assisted String name, + Sequences sequences) { this.self = self; this.serverIdent = serverIdent; this.db = db; @@ -109,6 +112,7 @@ public class CreateGroup implements RestModifyView this.systemGroupBackend = systemGroupBackend; this.defaultVisibleToAll = cfg.getBoolean("groups", "newGroupsVisibleToAll", false); this.name = name; + this.sequences = sequences; } public CreateGroup addOption(ListGroupsOption o) { @@ -193,7 +197,7 @@ public class CreateGroup implements RestModifyView } } - AccountGroup.Id groupId = new AccountGroup.Id(db.nextAccountGroupId()); + AccountGroup.Id groupId = new AccountGroup.Id(sequences.nextGroupId()); AccountGroup.UUID uuid = GroupUUID.make( createGroupArgs.getGroupName(), diff --git a/java/com/google/gerrit/server/schema/SchemaCreator.java b/java/com/google/gerrit/server/schema/SchemaCreator.java index 7ac7721cfb..333ee0d006 100644 --- a/java/com/google/gerrit/server/schema/SchemaCreator.java +++ b/java/com/google/gerrit/server/schema/SchemaCreator.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupName; @@ -26,7 +27,9 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.SystemConfig; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.Sequences; import com.google.gerrit.server.account.GroupUUID; +import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePath; @@ -41,6 +44,7 @@ import com.google.gerrit.server.group.db.InternalGroupCreation; import com.google.gerrit.server.group.db.InternalGroupUpdate; import com.google.gerrit.server.index.group.GroupIndex; import com.google.gerrit.server.index.group.GroupIndexCollection; +import com.google.gerrit.server.notedb.NotesMigration; import com.google.gwtorm.jdbc.JdbcExecutor; import com.google.gwtorm.jdbc.JdbcSchema; import com.google.gwtorm.server.OrmDuplicateKeyException; @@ -67,6 +71,11 @@ public class SchemaCreator { private final GroupIndexCollection indexCollection; private final boolean writeGroupsToNoteDb; + private final Config config; + private final MetricMaker metricMaker; + private final NotesMigration migration; + private final AllProjectsName allProjectsName; + @Inject public SchemaCreator( SitePaths site, @@ -77,8 +86,23 @@ public class SchemaCreator { @GerritPersonIdent PersonIdent au, DataSourceType dst, GroupIndexCollection ic, - @GerritServerConfig Config config) { - this(site.site_path, repoManager, ap, auc, allUsersName, au, dst, ic, config); + @GerritServerConfig Config config, + MetricMaker metricMaker, + NotesMigration migration, + AllProjectsName apName) { + this( + site.site_path, + repoManager, + ap, + auc, + allUsersName, + au, + dst, + ic, + config, + metricMaker, + migration, + apName); } public SchemaCreator( @@ -90,7 +114,10 @@ public class SchemaCreator { @GerritPersonIdent PersonIdent au, DataSourceType dst, GroupIndexCollection ic, - Config config) { + Config config, + MetricMaker metricMaker, + NotesMigration migration, + AllProjectsName apName) { site_path = site; this.repoManager = repoManager; allProjectsCreator = ap; @@ -104,6 +131,11 @@ public class SchemaCreator { // Don't flip this flag in a production setting! We only added it to spread the implementation // of groups in NoteDb among several changes which are gradually merged. writeGroupsToNoteDb = config.getBoolean("user", null, "writeGroupsToNoteDb", false); + + this.config = config; + this.allProjectsName = apName; + this.migration = migration; + this.metricMaker = metricMaker; } public void create(ReviewDb db) throws OrmException, IOException, ConfigInvalidException { @@ -124,17 +156,29 @@ public class SchemaCreator { // We have to create the All-Users repository before we can use it to store the groups in it. allUsersCreator.setAdministrators(admins).create(); + // Don't rely on injection to construct Sequences, as it requires ReviewDb. + Sequences seqs = + new Sequences( + config, + () -> db, + migration, + repoManager, + GitReferenceUpdated.DISABLED, + allProjectsName, + allUsersName, + metricMaker); try (Repository repository = repoManager.openRepository(allUsersName)) { - createAdminsGroup(db, repository, admins); - createBatchUsersGroup(db, repository, batchUsers, admins); + createAdminsGroup(db, seqs, repository, admins); + createBatchUsersGroup(db, seqs, repository, batchUsers, admins); } dataSourceType.getIndexScript().run(db); } - private void createAdminsGroup(ReviewDb db, Repository repository, GroupReference groupReference) + private void createAdminsGroup( + ReviewDb db, Sequences seqs, Repository repository, GroupReference groupReference) throws OrmException, IOException, ConfigInvalidException { - InternalGroupCreation groupCreation = getGroupCreation(db, groupReference); + InternalGroupCreation groupCreation = getGroupCreation(seqs, groupReference); InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder().setDescription("Gerrit Site Administrators").build(); @@ -142,9 +186,13 @@ public class SchemaCreator { } private void createBatchUsersGroup( - ReviewDb db, Repository repository, GroupReference groupReference, GroupReference admins) + ReviewDb db, + Sequences seqs, + Repository repository, + GroupReference groupReference, + GroupReference admins) throws OrmException, IOException, ConfigInvalidException { - InternalGroupCreation groupCreation = getGroupCreation(db, groupReference); + InternalGroupCreation groupCreation = getGroupCreation(seqs, groupReference); InternalGroupUpdate groupUpdate = InternalGroupUpdate.builder() .setDescription("Users who perform batch actions on Gerrit") @@ -222,11 +270,12 @@ public class SchemaCreator { return new GroupReference(groupUuid, name); } - private static InternalGroupCreation getGroupCreation(ReviewDb db, GroupReference groupReference) + private InternalGroupCreation getGroupCreation(Sequences seqs, GroupReference groupReference) throws OrmException { + int next = seqs.nextGroupId(); return InternalGroupCreation.builder() .setNameKey(new AccountGroup.NameKey(groupReference.getName())) - .setId(new AccountGroup.Id(db.nextAccountGroupId())) + .setId(new AccountGroup.Id(next)) .setGroupUUID(groupReference.getUUID()) .setCreatedOn(TimeUtil.nowTs()) .build(); diff --git a/java/com/google/gerrit/server/schema/SchemaVersion.java b/java/com/google/gerrit/server/schema/SchemaVersion.java index bf5acbd2d3..2cb57a81f8 100644 --- a/java/com/google/gerrit/server/schema/SchemaVersion.java +++ b/java/com/google/gerrit/server/schema/SchemaVersion.java @@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit; /** A version of the database schema. */ public abstract class SchemaVersion { /** The current schema version. */ - public static final Class C = Schema_162.class; + public static final Class C = Schema_163.class; public static int getBinaryVersion() { return guessVersion(C); diff --git a/java/com/google/gerrit/server/schema/Schema_163.java b/java/com/google/gerrit/server/schema/Schema_163.java new file mode 100644 index 0000000000..b0a52ffca3 --- /dev/null +++ b/java/com/google/gerrit/server/schema/Schema_163.java @@ -0,0 +1,57 @@ +// Copyright (C) 2017 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.schema; + +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.Sequences; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.notedb.RepoSequence; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import java.sql.SQLException; + +/** Create group sequence in NoteDb */ +public class Schema_163 extends SchemaVersion { + private final GitRepositoryManager repoManager; + private final AllUsersName allUsersName; + + @Inject + Schema_163( + Provider prior, GitRepositoryManager repoManager, AllUsersName allUsersName) { + super(prior); + this.repoManager = repoManager; + this.allUsersName = allUsersName; + } + + @Override + protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException, SQLException { + @SuppressWarnings("deprecation") + RepoSequence.Seed groupSeed = () -> db.nextAccountGroupId(); + RepoSequence groupSeq = + new RepoSequence( + repoManager, + GitReferenceUpdated.DISABLED, + allUsersName, + Sequences.NAME_GROUPS, + groupSeed, + 1); + + // consume one account ID to ensure that the group sequence is initialized in NoteDb + groupSeq.next(); + } +} diff --git a/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java b/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java index ec8b423f98..bdcee402ce 100644 --- a/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java +++ b/javatests/com/google/gerrit/server/schema/SchemaUpdaterTest.java @@ -18,6 +18,8 @@ import static com.google.common.truth.Truth.assertThat; import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.lifecycle.LifecycleManager; +import com.google.gerrit.metrics.DisabledMetricMaker; +import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.reviewdb.client.SystemConfig; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.GerritPersonIdent; @@ -113,6 +115,7 @@ public class SchemaUpdaterTest { bind(SystemGroupBackend.class); install(new NotesMigration.Module()); + bind(MetricMaker.class).to(DisabledMetricMaker.class); } }) .getInstance(SchemaUpdater.class);