diff --git a/Documentation/intro-rockstar.txt b/Documentation/intro-rockstar.txt index b60a91fc73..0b679502a6 100644 --- a/Documentation/intro-rockstar.txt +++ b/Documentation/intro-rockstar.txt @@ -60,7 +60,7 @@ older (imperfect) revision is not lost. It can be found via the `git reflog`. At least two well-known open source projects insist on these practices: * link:http://git-scm.com/[Git] -* link:http://www.kernel.org/category/about.html/[Linux Kernel] +* link:http://www.kernel.org/category/about.html[Linux Kernel] However, contributors to these projects don’t refine and polish their changes in private until they’re perfect. Instead, polishing code is part of a review diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index a994711e22..ad5ea8205c 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2467,7 +2467,7 @@ As response a list of link:#change-message-info[ChangeMessageInfo] entities is r Retrieves a change message including link:#detailed-accounts[detailed account information]. -- -'GET /changes/link:#change-id[\{change-id\}]/message/link:#change-message-id[\{change-message-id\}]' +'GET /changes/link:#change-id[\{change-id\}]/messages/link:#change-message-id[\{change-message-id\}]' -- As response a link:#change-message-info[ChangeMessageInfo] entity is returned. @@ -2496,8 +2496,8 @@ As response a link:#change-message-info[ChangeMessageInfo] entity is returned. [[delete-change-message]] === Delete Change Message -- -'DELETE /changes/link:#change-id[\{change-id\}]/message/link:#change-message-id[\{change-message-id\}]' + -'POST /changes/link:#change-id[\{change-id\}]//message/link:#change-message-id[\{change-message-id\}]/delete' +'DELETE /changes/link:#change-id[\{change-id\}]/messages/link:#change-message-id[\{change-message-id\}]' + +'POST /changes/link:#change-id[\{change-id\}]/messages/link:#change-message-id[\{change-message-id\}]/delete' -- Deletes a change message by replacing the change message with a new message, @@ -2513,14 +2513,14 @@ To delete a change message, send a DELETE request: .Request ---- - DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780 HTTP/1.0 + DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/messages/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780 HTTP/1.0 ---- To provide a reason for the deletion, use a POST request: .Request ---- - POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780/delete HTTP/1.0 + POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/messages/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780/delete HTTP/1.0 Content-Type: application/json; charset=UTF-8 { diff --git a/java/com/google/gerrit/acceptance/TestAccount.java b/java/com/google/gerrit/acceptance/TestAccount.java index 5ce44ffda2..ac197bdb8a 100644 --- a/java/com/google/gerrit/acceptance/TestAccount.java +++ b/java/com/google/gerrit/acceptance/TestAccount.java @@ -16,6 +16,7 @@ package com.google.gerrit.acceptance; import static java.util.stream.Collectors.toList; +import com.google.common.base.MoreObjects; import com.google.common.net.InetAddresses; import com.google.gerrit.mail.Address; import com.google.gerrit.reviewdb.client.Account; @@ -71,4 +72,9 @@ public class TestAccount { public Account.Id getId() { return id; } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("id", id).add("username", username).toString(); + } } diff --git a/java/com/google/gerrit/extensions/restapi/RestApiException.java b/java/com/google/gerrit/extensions/restapi/RestApiException.java index 28398a412c..b09723e8df 100644 --- a/java/com/google/gerrit/extensions/restapi/RestApiException.java +++ b/java/com/google/gerrit/extensions/restapi/RestApiException.java @@ -14,7 +14,7 @@ package com.google.gerrit.extensions.restapi; -/** Root exception type for JSON API failures. */ +/** Root exception type for REST API failures. */ public class RestApiException extends Exception { private static final long serialVersionUID = 1L; private CacheControl caching = CacheControl.NONE; diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java index 76e7f81ad7..41d02f507b 100644 --- a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java +++ b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java @@ -656,9 +656,12 @@ public class ExternalIdNotes extends VersionedMetaData { @Override protected void onLoad() throws IOException, ConfigInvalidException { - logger.atFine().log("Reading external ID note map"); - - noteMap = revision != null ? NoteMap.read(reader, revision) : NoteMap.newEmptyMap(); + if (revision != null) { + logger.atFine().log("Reading external ID note map"); + noteMap = NoteMap.read(reader, revision); + } else { + noteMap = NoteMap.newEmptyMap(); + } if (afterReadRevision != null) { afterReadRevision.run(); diff --git a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java index b33aa3c222..f1095709b1 100644 --- a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java +++ b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java @@ -17,10 +17,11 @@ package com.google.gerrit.server.git.meta; import static com.google.common.base.Preconditions.checkArgument; import com.google.common.base.MoreObjects; -import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.git.LockFailureException; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.logging.TraceContext; +import com.google.gerrit.server.logging.TraceContext.TraceTimer; import java.io.BufferedReader; import java.io.IOException; import java.io.StringReader; @@ -64,8 +65,6 @@ import org.eclipse.jgit.util.RawParseUtils; * read from the repository, or format an update that can later be written back to the repository. */ public abstract class VersionedMetaData { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - /** * Path information that does not hold references to any repository data structures, allowing the * application to retain this object for long periods of time. @@ -497,10 +496,11 @@ public abstract class VersionedMetaData { return new byte[] {}; } - logger.atFine().log( - "Read file '%s' from ref '%s' of project '%s' from revision '%s'", - fileName, getRefName(), projectName, revision.name()); - try (TreeWalk tw = TreeWalk.forPath(reader, fileName, revision.getTree())) { + try (TraceTimer timer = + TraceContext.newTimer( + "Read file '%s' from ref '%s' of project '%s' from revision '%s'", + fileName, getRefName(), projectName, revision.name()); + TreeWalk tw = TreeWalk.forPath(reader, fileName, revision.getTree())) { if (tw != null) { ObjectLoader obj = reader.open(tw.getObjectId(0), Constants.OBJ_BLOB); return obj.getCachedBytes(Integer.MAX_VALUE); @@ -572,22 +572,24 @@ public abstract class VersionedMetaData { } protected void saveFile(String fileName, byte[] raw) throws IOException { - logger.atFine().log( - "Save file '%s' in ref '%s' of project '%s'", fileName, getRefName(), projectName); - DirCacheEditor editor = newTree.editor(); - if (raw != null && 0 < raw.length) { - final ObjectId blobId = inserter.insert(Constants.OBJ_BLOB, raw); - editor.add( - new PathEdit(fileName) { - @Override - public void apply(DirCacheEntry ent) { - ent.setFileMode(FileMode.REGULAR_FILE); - ent.setObjectId(blobId); - } - }); - } else { - editor.add(new DeletePath(fileName)); + try (TraceTimer timer = + TraceContext.newTimer( + "Save file '%s' in ref '%s' of project '%s'", fileName, getRefName(), projectName)) { + DirCacheEditor editor = newTree.editor(); + if (raw != null && 0 < raw.length) { + final ObjectId blobId = inserter.insert(Constants.OBJ_BLOB, raw); + editor.add( + new PathEdit(fileName) { + @Override + public void apply(DirCacheEntry ent) { + ent.setFileMode(FileMode.REGULAR_FILE); + ent.setObjectId(blobId); + } + }); + } else { + editor.add(new DeletePath(fileName)); + } + editor.finish(); } - editor.finish(); } } diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java index 77af248398..194258e89e 100644 --- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java +++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java @@ -19,6 +19,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.git.LockFailureException; @@ -40,6 +41,8 @@ import com.google.gerrit.server.git.meta.MetaDataUpdate; import com.google.gerrit.server.group.GroupAuditService; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.index.group.GroupIndexer; +import com.google.gerrit.server.logging.TraceContext; +import com.google.gerrit.server.logging.TraceContext.TraceTimer; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.util.time.TimeUtil; import com.google.gwtorm.server.OrmDuplicateKeyException; @@ -95,6 +98,8 @@ public class GroupsUpdate { GroupsUpdate createWithServerIdent(); } + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; private final GroupCache groupCache; @@ -258,10 +263,14 @@ public class GroupsUpdate { public InternalGroup createGroup( InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) throws OrmDuplicateKeyException, IOException, ConfigInvalidException { - InternalGroup createdGroup = createGroupInNoteDbWithRetry(groupCreation, groupUpdate); - updateCachesOnGroupCreation(createdGroup); - dispatchAuditEventsOnGroupCreation(createdGroup); - return createdGroup; + try (TraceTimer timer = + TraceContext.newTimer( + "Creating group '%s'", groupUpdate.getName().orElseGet(groupCreation::getNameKey))) { + InternalGroup createdGroup = createGroupInNoteDbWithRetry(groupCreation, groupUpdate); + evictCachesOnGroupCreation(createdGroup); + dispatchAuditEventsOnGroupCreation(createdGroup); + return createdGroup; + } } /** @@ -276,16 +285,18 @@ public class GroupsUpdate { */ public void updateGroup(AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) throws OrmDuplicateKeyException, IOException, NoSuchGroupException, ConfigInvalidException { - Optional updatedOn = groupUpdate.getUpdatedOn(); - if (!updatedOn.isPresent()) { - updatedOn = Optional.of(TimeUtil.nowTs()); - groupUpdate = groupUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build(); - } + try (TraceTimer timer = TraceContext.newTimer("Updating group %s", groupUuid)) { + Optional updatedOn = groupUpdate.getUpdatedOn(); + if (!updatedOn.isPresent()) { + updatedOn = Optional.of(TimeUtil.nowTs()); + groupUpdate = groupUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build(); + } - UpdateResult result = updateGroupInNoteDbWithRetry(groupUuid, groupUpdate); - updateNameInProjectConfigsIfNecessary(result); - updateCachesOnGroupUpdate(result); - dispatchAuditEventsOnGroupUpdate(result, updatedOn.get()); + UpdateResult result = updateGroupInNoteDbWithRetry(groupUuid, groupUpdate); + updateNameInProjectConfigsIfNecessary(result); + evictCachesOnGroupUpdate(result); + dispatchAuditEventsOnGroupUpdate(result, updatedOn.get()); + } } private InternalGroup createGroupInNoteDbWithRetry( @@ -424,7 +435,8 @@ public class GroupsUpdate { allUsersName, batchRefUpdate, currentUser.map(user -> user.state()).orElse(null)); } - private void updateCachesOnGroupCreation(InternalGroup createdGroup) throws IOException { + private void evictCachesOnGroupCreation(InternalGroup createdGroup) throws IOException { + logger.atFine().log("evict caches on creation of group %s", createdGroup.getGroupUUID()); // By UUID is used for the index and hence should be evicted before refreshing the index. groupCache.evict(createdGroup.getGroupUUID()); indexer.get().index(createdGroup.getGroupUUID()); @@ -436,7 +448,8 @@ public class GroupsUpdate { createdGroup.getSubgroups().forEach(groupIncludeCache::evictParentGroupsOf); } - private void updateCachesOnGroupUpdate(UpdateResult result) throws IOException { + private void evictCachesOnGroupUpdate(UpdateResult result) throws IOException { + logger.atFine().log("evict caches on update of group %s", result.getGroupUuid()); // By UUID is used for the index and hence should be evicted before refreshing the index. groupCache.evict(result.getGroupUuid()); indexer.get().index(result.getGroupUuid()); diff --git a/java/com/google/gerrit/server/logging/TraceContext.java b/java/com/google/gerrit/server/logging/TraceContext.java index ded34d03e8..5c0406ddd1 100644 --- a/java/com/google/gerrit/server/logging/TraceContext.java +++ b/java/com/google/gerrit/server/logging/TraceContext.java @@ -206,6 +206,23 @@ public class TraceContext implements AutoCloseable { return new TraceTimer(format, arg1, arg2, arg3); } + /** + * Opens a new timer that logs the time for an operation if request tracing is enabled. + * + *

If request tracing is not enabled this is a no-op. + * + * @param format the message format string + * @param arg1 first argument for the message + * @param arg2 second argument for the message + * @param arg3 third argument for the message + * @param arg4 fourth argument for the message + * @return the trace timer + */ + public static TraceTimer newTimer( + String format, Object arg1, Object arg2, Object arg3, Object arg4) { + return new TraceTimer(format, arg1, arg2, arg3, arg4); + } + public static class TraceTimer implements AutoCloseable { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @@ -229,6 +246,16 @@ public class TraceContext implements AutoCloseable { this(elapsedMs -> logger.atFine().log(format + " (%d ms)", arg1, arg2, arg3, elapsedMs)); } + private TraceTimer( + String format, + @Nullable Object arg1, + @Nullable Object arg2, + @Nullable Object arg3, + @Nullable Object arg4) { + this( + elapsedMs -> logger.atFine().log(format + " (%d ms)", arg1, arg2, arg3, arg4, elapsedMs)); + } + private TraceTimer(Consumer logFn) { this.logFn = logFn; this.stopwatch = Stopwatch.createStarted();