From 2605168713139277fafa809f9fe0f98f4715f117 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 22 Dec 2015 19:08:40 +0100 Subject: [PATCH] User memoized supplier pattern to reduce resource consumption It's pointless to retrieve the whole data from the database and git backends when it's thrown away anyway. Use memoized supplier pattern to only retrieve the data if really needed. We need memoization in this context to ensure, that when the data is retrieved, it is done only once and not multiple times in different plugins and in hooks firing tool chain. This change breaks plugins that consume change events. However, given that only few plugins, e.g. reviewers, are affected by this change, this API change is justfied, as it's substantially decreasing the load on the gerrit site. One side effect of this change is stream event JSON serialization. Use custom JSON serializer to resolve supplier with their content. Test Plan: Non retrieval of data wth default gerrit install + reviewers plugin. 1. install gerrit with all core plugins 2. install reviewers plugin 3. create a group_100 that contains 100 users 4. add a reviewer to a change and select the group_100 Without this change hundreds of SQL select statements are executed in vain. That's because 100 ChangeEvents instances are created and multiple SQL select statements are executed per instance. With this change exact 0 select SQL statements are executed. Memoized suppliers are created but not used. Change-Id: I10e257be37ff60276e4187659926fe32e5be136b Reported-By: Luca Milanesio --- .../rest/change/AbstractSubmit.java | 2 +- .../gerrit/common/ChangeHookRunner.java | 487 ++++++++++++------ .../server/events/ChangeAbandonedEvent.java | 3 +- .../gerrit/server/events/ChangeEvent.java | 9 +- .../server/events/ChangeMergedEvent.java | 3 +- .../server/events/ChangeRestoredEvent.java | 3 +- .../server/events/CommentAddedEvent.java | 5 +- .../server/events/DraftPublishedEvent.java | 3 +- .../gerrit/server/events/EventFactory.java | 36 +- .../server/events/HashtagsChangedEvent.java | 3 +- .../server/events/MergeFailedEvent.java | 3 +- .../server/events/PatchSetCreatedEvent.java | 3 +- .../gerrit/server/events/PatchSetEvent.java | 3 +- .../gerrit/server/events/RefUpdatedEvent.java | 9 +- .../server/events/ReviewerAddedEvent.java | 3 +- .../server/events/TopicChangedEvent.java | 3 +- .../gerrit/sshd/commands/StreamEvents.java | 21 +- 17 files changed, 418 insertions(+), 181 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index ef3faa5555..2bd093d2c8 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -106,7 +106,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { public void onEvent(Event event) { if (event instanceof ChangeMergedEvent) { ChangeMergedEvent changeMergedEvent = (ChangeMergedEvent) event; - mergeResults.put(changeMergedEvent.change.number, + mergeResults.put(changeMergedEvent.change.get().number, changeMergedEvent.newRev); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index d9934f604a..8e02cc7211 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java @@ -14,6 +14,10 @@ package com.google.gerrit.common; +import com.google.common.base.Optional; +import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; +import com.google.common.base.Throwables; import com.google.common.collect.Sets; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.gerrit.common.data.LabelType; @@ -34,8 +38,11 @@ import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.data.AccountAttribute; import com.google.gerrit.server.data.ApprovalAttribute; +import com.google.gerrit.server.data.ChangeAttribute; import com.google.gerrit.server.data.PatchSetAttribute; +import com.google.gerrit.server.data.RefUpdateAttribute; import com.google.gerrit.server.events.ChangeAbandonedEvent; import com.google.gerrit.server.events.ChangeMergedEvent; import com.google.gerrit.server.events.ChangeRestoredEvent; @@ -174,46 +181,46 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, private final DynamicSet unrestrictedListeners; /** Path of the new patchset hook. */ - private final Path patchsetCreatedHook; + private final Optional patchsetCreatedHook; /** Path of the draft published hook. */ - private final Path draftPublishedHook; + private final Optional draftPublishedHook; /** Path of the new comments hook. */ - private final Path commentAddedHook; + private final Optional commentAddedHook; /** Path of the change merged hook. */ - private final Path changeMergedHook; + private final Optional changeMergedHook; /** Path of the merge failed hook. */ - private final Path mergeFailedHook; + private final Optional mergeFailedHook; /** Path of the change abandoned hook. */ - private final Path changeAbandonedHook; + private final Optional changeAbandonedHook; /** Path of the change restored hook. */ - private final Path changeRestoredHook; + private final Optional changeRestoredHook; /** Path of the ref updated hook. */ - private final Path refUpdatedHook; + private final Optional refUpdatedHook; /** Path of the reviewer added hook. */ - private final Path reviewerAddedHook; + private final Optional reviewerAddedHook; /** Path of the topic changed hook. */ - private final Path topicChangedHook; + private final Optional topicChangedHook; /** Path of the cla signed hook. */ - private final Path claSignedHook; + private final Optional claSignedHook; /** Path of the update hook. */ - private final Path refUpdateHook; + private final Optional refUpdateHook; /** Path of the hashtags changed hook */ - private final Path hashtagsChangedHook; + private final Optional hashtagsChangedHook; /** Path of the project created hook. */ - private final Path projectCreatedHook; + private final Optional projectCreatedHook; private final String anonymousCowardName; @@ -297,10 +304,11 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, .build()); } - private static Path hook(Config config, Path path, String name) { + private static Optional hook(Config config, Path path, String name) { String setting = name.replace("-", "") + "hook"; String value = config.getString("hooks", null, setting); - return path.resolve(value != null ? value : name); + Path p = path.resolve(value != null ? value : name); + return Files.exists(p) ? Optional.of(p) : Optional.absent(); } @Override @@ -335,16 +343,6 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, } } - private PatchSetAttribute asPatchSetAttribute(Change change, - PatchSet patchSet, ReviewDb db) throws OrmException { - try (Repository repo = repoManager.openRepository(change.getProject()); - RevWalk revWalk = new RevWalk(repo)) { - return eventFactory.asPatchSetAttribute(db, revWalk, patchSet); - } catch (IOException e) { - throw new OrmException(e); - } - } - /** * Fire the update hook * @@ -352,6 +350,9 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, @Override public HookResult doRefUpdateHook(Project project, String refname, Account uploader, ObjectId oldId, ObjectId newId) { + if (!refUpdateHook.isPresent()) { + return null; + } List args = new ArrayList<>(); addArg(args, "--project", project.getName()); @@ -368,8 +369,13 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, ProjectCreatedEvent event = new ProjectCreatedEvent(); event.projectName = project.get(); event.headName = headName; + fireEvent(project, event); + if (!projectCreatedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); addArg(args, "--project", project.get()); addArg(args, "--head", headName); @@ -382,32 +388,42 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, * * @param change The change itself. * @param patchSet The Patchset that was created. + * @param db Review database. * @throws OrmException */ @Override - public void doPatchsetCreatedHook(Change change, PatchSet patchSet, - ReviewDb db) throws OrmException { + public void doPatchsetCreatedHook(Change change, + PatchSet patchSet, ReviewDb db) throws OrmException { PatchSetCreatedEvent event = new PatchSetCreatedEvent(); - AccountState uploader = accountCache.get(patchSet.getUploader()); - AccountState owner = accountCache.get(change.getOwner()); + Supplier uploader = + getAccountSupplier(patchSet.getUploader()); + Supplier owner = getAccountSupplier(change.getOwner()); + + event.change = changeAttributeSupplier(change); + event.patchSet = patchSetAttributeSupplier(change, patchSet); + event.uploader = accountAttributeSupplier(uploader); - event.change = eventFactory.asChangeAttribute(db, change); - event.patchSet = asPatchSetAttribute(change, patchSet, db); - event.uploader = eventFactory.asAccountAttribute(uploader.getAccount()); fireEvent(change, event, db); + if (!patchsetCreatedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); + + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + addArg(args, "--change", c.id); addArg(args, "--is-draft", String.valueOf(patchSet.isDraft())); - addArg(args, "--kind", String.valueOf(event.patchSet.kind)); - addArg(args, "--change-url", event.change.url); - addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); - addArg(args, "--uploader", getDisplayName(uploader.getAccount())); - addArg(args, "--commit", event.patchSet.revision); - addArg(args, "--patchset", event.patchSet.number); + addArg(args, "--kind", String.valueOf(ps.kind)); + addArg(args, "--change-url", c.url); + addArg(args, "--change-owner", getDisplayName(owner.get().getAccount())); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); + addArg(args, "--uploader", getDisplayName(uploader.get().getAccount())); + addArg(args, "--commit", ps.revision); + addArg(args, "--patchset", ps.number); runHook(change.getProject(), patchsetCreatedHook, args); } @@ -416,62 +432,88 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, public void doDraftPublishedHook(Change change, PatchSet patchSet, ReviewDb db) throws OrmException { DraftPublishedEvent event = new DraftPublishedEvent(); - AccountState uploader = accountCache.get(patchSet.getUploader()); - AccountState owner = accountCache.get(change.getOwner()); + Supplier uploader = + getAccountSupplier(patchSet.getUploader()); + Supplier owner = getAccountSupplier(change.getOwner()); + + event.change = changeAttributeSupplier(change); + event.patchSet = patchSetAttributeSupplier(change, patchSet); + event.uploader = accountAttributeSupplier(uploader); - event.change = eventFactory.asChangeAttribute(db, change); - event.patchSet = asPatchSetAttribute(change, patchSet, db); - event.uploader = eventFactory.asAccountAttribute(uploader.getAccount()); fireEvent(change, event, db); + if (!draftPublishedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); - addArg(args, "--change-url", event.change.url); - addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); - addArg(args, "--uploader", getDisplayName(uploader.getAccount())); - addArg(args, "--commit", event.patchSet.revision); - addArg(args, "--patchset", event.patchSet.number); + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + + addArg(args, "--change", c.id); + addArg(args, "--change-url", c.url); + addArg(args, "--change-owner", getDisplayName(owner.get().getAccount())); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); + addArg(args, "--uploader", getDisplayName(uploader.get().getAccount())); + addArg(args, "--commit", ps.revision); + addArg(args, "--patchset", ps.number); runHook(change.getProject(), draftPublishedHook, args); } @Override - public void doCommentAddedHook(Change change, Account account, - PatchSet patchSet, String comment, Map approvals, + public void doCommentAddedHook(final Change change, Account account, + PatchSet patchSet, String comment, final Map approvals, ReviewDb db) throws OrmException { CommentAddedEvent event = new CommentAddedEvent(); - AccountState owner = accountCache.get(change.getOwner()); + Supplier owner = getAccountSupplier(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.author = eventFactory.asAccountAttribute(account); - event.patchSet = asPatchSetAttribute(change, patchSet, db); + event.change = changeAttributeSupplier(change); + event.author = accountAttributeSupplier(account); + event.patchSet = patchSetAttributeSupplier(change, patchSet); event.comment = comment; - - LabelTypes labelTypes = projectCache.get(change.getProject()).getLabelTypes(); - if (approvals.size() > 0) { - event.approvals = new ApprovalAttribute[approvals.size()]; - int i = 0; - for (Map.Entry approval : approvals.entrySet()) { - event.approvals[i++] = getApprovalAttribute(labelTypes, approval); - } - } + event.approvals = Suppliers.memoize( + new Supplier() { + @Override + public ApprovalAttribute[] get() { + LabelTypes labelTypes = projectCache.get( + change.getProject()).getLabelTypes(); + if (approvals.size() > 0) { + ApprovalAttribute[] r = new ApprovalAttribute[approvals.size()]; + int i = 0; + for (Map.Entry approval : approvals.entrySet()) { + r[i++] = getApprovalAttribute(labelTypes, approval); + } + return r; + } + return null; + } + }); fireEvent(change, event, db); + if (!commentAddedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + + addArg(args, "--change", c.id); addArg(args, "--is-draft", patchSet.isDraft() ? "true" : "false"); - addArg(args, "--change-url", event.change.url); - addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); + addArg(args, "--change-url", c.url); + addArg(args, "--change-owner", getDisplayName(owner.get().getAccount())); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); addArg(args, "--author", getDisplayName(account)); - addArg(args, "--commit", event.patchSet.revision); + addArg(args, "--commit", ps.revision); addArg(args, "--comment", comment == null ? "" : comment); + LabelTypes labelTypes = projectCache.get( + change.getProject()).getLabelTypes(); for (Map.Entry approval : approvals.entrySet()) { LabelType lt = labelTypes.byLabel(approval.getKey()); if (lt != null) { @@ -487,23 +529,31 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, PatchSet patchSet, ReviewDb db, String mergeResultRev) throws OrmException { ChangeMergedEvent event = new ChangeMergedEvent(); - AccountState owner = accountCache.get(change.getOwner()); + Supplier owner = getAccountSupplier(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.submitter = eventFactory.asAccountAttribute(account); - event.patchSet = asPatchSetAttribute(change, patchSet, db); + event.change = changeAttributeSupplier(change); + event.submitter = accountAttributeSupplier(account); + event.patchSet = patchSetAttributeSupplier(change, patchSet); event.newRev = mergeResultRev; + fireEvent(change, event, db); + if (!changeMergedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); - addArg(args, "--change-url", event.change.url); - addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + + addArg(args, "--change", c.id); + addArg(args, "--change-url", c.url); + addArg(args, "--change-owner", getDisplayName(owner.get().getAccount())); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); addArg(args, "--submitter", getDisplayName(account)); - addArg(args, "--commit", event.patchSet.revision); + addArg(args, "--commit", ps.revision); addArg(args, "--newrev", mergeResultRev); runHook(change.getProject(), changeMergedHook, args); @@ -514,23 +564,31 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, PatchSet patchSet, String reason, ReviewDb db) throws OrmException { MergeFailedEvent event = new MergeFailedEvent(); - AccountState owner = accountCache.get(change.getOwner()); + Supplier owner = getAccountSupplier(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.submitter = eventFactory.asAccountAttribute(account); - event.patchSet = asPatchSetAttribute(change, patchSet, db); + event.change = changeAttributeSupplier(change); + event.submitter = accountAttributeSupplier(account); + event.patchSet = patchSetAttributeSupplier(change, patchSet); event.reason = reason; + fireEvent(change, event, db); + if (!mergeFailedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); - addArg(args, "--change-url", event.change.url); - addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + + addArg(args, "--change", c.id); + addArg(args, "--change-url", c.url); + addArg(args, "--change-owner", getDisplayName(owner.get().getAccount())); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); addArg(args, "--submitter", getDisplayName(account)); - addArg(args, "--commit", event.patchSet.revision); + addArg(args, "--commit", ps.revision); addArg(args, "--reason", reason == null ? "" : reason); runHook(change.getProject(), mergeFailedHook, args); @@ -543,21 +601,29 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, ChangeAbandonedEvent event = new ChangeAbandonedEvent(); AccountState owner = accountCache.get(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.abandoner = eventFactory.asAccountAttribute(account); - event.patchSet = asPatchSetAttribute(change, patchSet, db); + event.change = changeAttributeSupplier(change); + event.abandoner = accountAttributeSupplier(account); + event.patchSet = patchSetAttributeSupplier(change, patchSet); event.reason = reason; + fireEvent(change, event, db); + if (!changeAbandonedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); - addArg(args, "--change-url", event.change.url); + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + + addArg(args, "--change", c.id); + addArg(args, "--change-url", c.url); addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); addArg(args, "--abandoner", getDisplayName(account)); - addArg(args, "--commit", event.patchSet.revision); + addArg(args, "--commit", ps.revision); addArg(args, "--reason", reason == null ? "" : reason); runHook(change.getProject(), changeAbandonedHook, args); @@ -570,21 +636,29 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, ChangeRestoredEvent event = new ChangeRestoredEvent(); AccountState owner = accountCache.get(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.restorer = eventFactory.asAccountAttribute(account); - event.patchSet = asPatchSetAttribute(change, patchSet, db); + event.change = changeAttributeSupplier(change); + event.restorer = accountAttributeSupplier(account); + event.patchSet = patchSetAttributeSupplier(change, patchSet); event.reason = reason; + fireEvent(change, event, db); + if (!changeRestoredHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); - addArg(args, "--change-url", event.change.url); + ChangeAttribute c = event.change.get(); + PatchSetAttribute ps = event.patchSet.get(); + + addArg(args, "--change", c.id); + addArg(args, "--change-url", c.url); addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); - addArg(args, "--topic", event.change.topic); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); + addArg(args, "--topic", c.topic); addArg(args, "--restorer", getDisplayName(account)); - addArg(args, "--commit", event.patchSet.revision); + addArg(args, "--commit", ps.revision); addArg(args, "--reason", reason == null ? "" : reason); runHook(change.getProject(), changeRestoredHook, args); @@ -598,21 +672,33 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, } @Override - public void doRefUpdatedHook(Branch.NameKey refName, ObjectId oldId, - ObjectId newId, Account account) { + public void doRefUpdatedHook(final Branch.NameKey refName, + final ObjectId oldId, final ObjectId newId, Account account) { RefUpdatedEvent event = new RefUpdatedEvent(); if (account != null) { - event.submitter = eventFactory.asAccountAttribute(account); + event.submitter = accountAttributeSupplier(account); } - event.refUpdate = eventFactory.asRefUpdateAttribute(oldId, newId, refName); + event.refUpdate = Suppliers.memoize( + new Supplier() { + @Override + public RefUpdateAttribute get() { + return eventFactory.asRefUpdateAttribute(oldId, newId, refName); + } + }); + fireEvent(refName, event); + if (!refUpdatedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--oldrev", event.refUpdate.oldRev); - addArg(args, "--newrev", event.refUpdate.newRev); - addArg(args, "--refname", event.refUpdate.refName); - addArg(args, "--project", event.refUpdate.project); + RefUpdateAttribute r = event.refUpdate.get(); + addArg(args, "--oldrev", r.oldRev); + addArg(args, "--newrev", r.newRev); + addArg(args, "--refname", r.refName); + addArg(args, "--project", r.project); if (account != null) { addArg(args, "--submitter", getDisplayName(account)); } @@ -624,19 +710,26 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, public void doReviewerAddedHook(Change change, Account account, PatchSet patchSet, ReviewDb db) throws OrmException { ReviewerAddedEvent event = new ReviewerAddedEvent(); - AccountState owner = accountCache.get(change.getOwner()); + Supplier owner = getAccountSupplier(change.getOwner()); + + event.change = changeAttributeSupplier(change); + event.patchSet = null;//patchSetAttributeSupplier(change, patchSet, db); + event.reviewer = null;//accountAttributeSupplier(account); - event.change = eventFactory.asChangeAttribute(db, change); - event.patchSet = asPatchSetAttribute(change, patchSet, db); - event.reviewer = eventFactory.asAccountAttribute(account); fireEvent(change, event, db); + if (!reviewerAddedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); - addArg(args, "--change-url", event.change.url); - addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); + ChangeAttribute c = event.change.get(); + + addArg(args, "--change", c.id); + addArg(args, "--change-url", c.url); + addArg(args, "--change-owner", getDisplayName(owner.get().getAccount())); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); addArg(args, "--reviewer", getDisplayName(account)); runHook(change.getProject(), reviewerAddedHook, args); @@ -649,19 +742,26 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, TopicChangedEvent event = new TopicChangedEvent(); AccountState owner = accountCache.get(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.changer = eventFactory.asAccountAttribute(account); + event.change = changeAttributeSupplier(change); + event.changer = accountAttributeSupplier(account); event.oldTopic = oldTopic; + fireEvent(change, event, db); + if (!topicChangedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); + ChangeAttribute c = event.change.get(); + + addArg(args, "--change", c.id); addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); addArg(args, "--changer", getDisplayName(account)); addArg(args, "--old-topic", oldTopic); - addArg(args, "--new-topic", event.change.topic); + addArg(args, "--new-topic", c.topic); runHook(change.getProject(), topicChangedHook, args); } @@ -681,19 +781,25 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, HashtagsChangedEvent event = new HashtagsChangedEvent(); AccountState owner = accountCache.get(change.getOwner()); - event.change = eventFactory.asChangeAttribute(db, change); - event.editor = eventFactory.asAccountAttribute(account); + event.change = changeAttributeSupplier(change); + event.editor = accountAttributeSupplier(account); event.hashtags = hashtagArray(hashtags); event.added = hashtagArray(added); event.removed = hashtagArray(removed); fireEvent(change, event, db); + if (!hashtagsChangedHook.isPresent()) { + return; + } + List args = new ArrayList<>(); - addArg(args, "--change", event.change.id); + ChangeAttribute c = event.change.get(); + + addArg(args, "--change", c.id); addArg(args, "--change-owner", getDisplayName(owner.getAccount())); - addArg(args, "--project", event.change.project); - addArg(args, "--branch", event.change.branch); + addArg(args, "--project", c.project); + addArg(args, "--branch", c.branch); addArg(args, "--editor", getDisplayName(account)); if (hashtags != null) { for (String hashtag : hashtags) { @@ -715,6 +821,10 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, @Override public void doClaSignupHook(Account account, String claName) { + if (!claSignedHook.isPresent()) { + return; + } + if (account != null) { List args = new ArrayList<>(); addArg(args, "--submitter", getDisplayName(account)); @@ -736,6 +846,67 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, fireEvent(branchName, event); } + private Supplier getAccountSupplier( + final Account.Id account) { + return Suppliers.memoize( + new Supplier() { + @Override + public AccountState get() { + return accountCache.get(account); + } + }); + } + + private Supplier accountAttributeSupplier( + final Supplier s) { + return Suppliers.memoize( + new Supplier() { + @Override + public AccountAttribute get() { + return eventFactory.asAccountAttribute(s.get().getAccount()); + } + }); + } + + private Supplier accountAttributeSupplier( + final Account account) { + return Suppliers.memoize( + new Supplier() { + @Override + public AccountAttribute get() { + return eventFactory.asAccountAttribute(account); + } + }); + } + + private Supplier patchSetAttributeSupplier( + final Change change, final PatchSet patchSet) { + return Suppliers.memoize( + new Supplier() { + @Override + public PatchSetAttribute get() { + try (Repository repo + = repoManager.openRepository(change.getProject()); + RevWalk revWalk = new RevWalk(repo)) { + return eventFactory.asPatchSetAttribute(revWalk, patchSet); + } catch (IOException e) { + throw Throwables.propagate(e); + } + } + }); + } + + private Supplier changeAttributeSupplier( + final Change change) { + return Suppliers.memoize( + new Supplier() { + @Override + public ChangeAttribute get() { + return eventFactory.asChangeAttribute(change); + } + }); + } + private void fireEventForUnrestrictedListeners(com.google.gerrit.server.events.Event event) { for (EventListener listener : unrestrictedListeners) { listener.onEvent(event); @@ -851,27 +1022,27 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, * @param hook the hook to execute. * @param args Arguments to use to run the hook. */ - private synchronized void runHook(Project.NameKey project, Path hook, + private synchronized void runHook(Project.NameKey project, Optional hook, List args) { - if (project != null && Files.exists(hook)) { - hookQueue.execute(new AsyncHookTask(project, hook, args)); + if (project != null && hook.isPresent()) { + hookQueue.execute(new AsyncHookTask(project, hook.get(), args)); } } - private synchronized void runHook(Path hook, List args) { - if (Files.exists(hook)) { - hookQueue.execute(new AsyncHookTask(null, hook, args)); + private synchronized void runHook(Optional hook, List args) { + if (hook.isPresent()) { + hookQueue.execute(new AsyncHookTask(null, hook.get(), args)); } } private HookResult runSyncHook(Project.NameKey project, - Path hook, List args) { + Optional hook, List args) { - if (!Files.exists(hook)) { + if (!hook.isPresent()) { return null; } - SyncHookTask syncHook = new SyncHookTask(project, hook, args); + SyncHookTask syncHook = new SyncHookTask(project, hook.get(), args); FutureTask task = new FutureTask<>(syncHook); syncHookThreadPool.execute(task); @@ -881,10 +1052,10 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher, try { return task.get(syncHookTimeout, TimeUnit.SECONDS); } catch (TimeoutException e) { - message = "Synchronous hook timed out " + hook.toAbsolutePath(); + message = "Synchronous hook timed out " + hook.get().toAbsolutePath(); log.error(message); } catch (Exception e) { - message = "Error running hook " + hook.toAbsolutePath(); + message = "Error running hook " + hook.get().toAbsolutePath(); log.error(message, e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeAbandonedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeAbandonedEvent.java index c285a82bcd..ea13d1f6b6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeAbandonedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeAbandonedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class ChangeAbandonedEvent extends PatchSetEvent { - public AccountAttribute abandoner; + public Supplier abandoner; public String reason; public ChangeAbandonedEvent() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeEvent.java index f083b7ab80..85cc998c4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeEvent.java @@ -14,13 +14,14 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.data.ChangeAttribute; public abstract class ChangeEvent extends RefEvent { - public ChangeAttribute change; + public Supplier change; protected ChangeEvent(String type) { super(type); @@ -28,15 +29,15 @@ public abstract class ChangeEvent extends RefEvent { @Override public Project.NameKey getProjectNameKey() { - return new Project.NameKey(change.project); + return new Project.NameKey(change.get().project); } @Override public String getRefName() { - return RefNames.fullName(change.branch); + return RefNames.fullName(change.get().branch); } public Change.Key getChangeKey() { - return new Change.Key(change.id); + return new Change.Key(change.get().id); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java index 41a95cb0f7..d762f0fe91 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class ChangeMergedEvent extends PatchSetEvent { - public AccountAttribute submitter; + public Supplier submitter; public String newRev; public ChangeMergedEvent() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeRestoredEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeRestoredEvent.java index a575a42cee..f045b8f15b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeRestoredEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeRestoredEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class ChangeRestoredEvent extends PatchSetEvent { - public AccountAttribute restorer; + public Supplier restorer; public String reason; public ChangeRestoredEvent () { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/CommentAddedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/CommentAddedEvent.java index 4391dfbdb9..9f0e0aa144 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/CommentAddedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/CommentAddedEvent.java @@ -14,12 +14,13 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; import com.google.gerrit.server.data.ApprovalAttribute; public class CommentAddedEvent extends PatchSetEvent { - public AccountAttribute author; - public ApprovalAttribute[] approvals; + public Supplier author; + public Supplier approvals; public String comment; public CommentAddedEvent() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/DraftPublishedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/DraftPublishedEvent.java index 5db628f673..f27f0f1cae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/DraftPublishedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/DraftPublishedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class DraftPublishedEvent extends PatchSetEvent { - public AccountAttribute uploader; + public Supplier uploader; public DraftPublishedEvent() { super("draft-published"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java index d52f8316d8..f160248070 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java @@ -61,6 +61,7 @@ import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -92,6 +93,7 @@ public class EventFactory { private final ApprovalsUtil approvalsUtil; private final ChangeKindCache changeKindCache; private final Provider queryProvider; + private final SchemaFactory schema; @Inject EventFactory(AccountCache accountCache, @@ -102,7 +104,8 @@ public class EventFactory { ChangeData.Factory changeDataFactory, ApprovalsUtil approvalsUtil, ChangeKindCache changeKindCache, - Provider queryProvider) { + Provider queryProvider, + SchemaFactory schema) { this.accountCache = accountCache; this.urlProvider = urlProvider; this.patchListCache = patchListCache; @@ -112,6 +115,7 @@ public class EventFactory { this.approvalsUtil = approvalsUtil; this.changeKindCache = changeKindCache; this.queryProvider = queryProvider; + this.schema = schema; } /** @@ -121,6 +125,23 @@ public class EventFactory { * @param change * @return object suitable for serialization to JSON */ + public ChangeAttribute asChangeAttribute(Change change) { + try (ReviewDb db = schema.open()) { + return asChangeAttribute(db, change); + } catch (OrmException e) { + log.error("Cannot open database connection", e); + return new ChangeAttribute(); + } + } + + /** + * Create a ChangeAttribute for the given change suitable for serialization to + * JSON. + * + * @param db Review database + * @param change + * @return object suitable for serialization to JSON + */ public ChangeAttribute asChangeAttribute(ReviewDb db, Change change) { ChangeAttribute a = new ChangeAttribute(); a.project = change.getProject().get(); @@ -429,6 +450,19 @@ public class EventFactory { * @param patchSet * @return object suitable for serialization to JSON */ + public PatchSetAttribute asPatchSetAttribute(RevWalk revWalk, + PatchSet patchSet) { + return asPatchSetAttribute(revWalk, patchSet); + } + + /** + * Create a PatchSetAttribute for the given patchset suitable for + * serialization to JSON. + * + * @param db Review database + * @param patchSet + * @return object suitable for serialization to JSON + */ public PatchSetAttribute asPatchSetAttribute(ReviewDb db, RevWalk revWalk, PatchSet patchSet) { PatchSetAttribute p = new PatchSetAttribute(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/HashtagsChangedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/HashtagsChangedEvent.java index c5919e50c9..28c6e4dead 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/HashtagsChangedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/HashtagsChangedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class HashtagsChangedEvent extends ChangeEvent { - public AccountAttribute editor; + public Supplier editor; public String[] added; public String[] removed; public String[] hashtags; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/MergeFailedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/MergeFailedEvent.java index 75cbcb028c..a7d8c457df 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/MergeFailedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/MergeFailedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class MergeFailedEvent extends PatchSetEvent { - public AccountAttribute submitter; + public Supplier submitter; public String reason; public MergeFailedEvent() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java index e4685939d5..4f7e3e73d5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetCreatedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class PatchSetCreatedEvent extends PatchSetEvent { - public AccountAttribute uploader; + public Supplier uploader; public PatchSetCreatedEvent() { super("patchset-created"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetEvent.java index cdaf601843..8d0b079b2a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/PatchSetEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.PatchSetAttribute; public class PatchSetEvent extends ChangeEvent { - public PatchSetAttribute patchSet; + public Supplier patchSet; protected PatchSetEvent(String type) { super(type); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/RefUpdatedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/RefUpdatedEvent.java index e5039ff425..f2a72b8eec 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/RefUpdatedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/RefUpdatedEvent.java @@ -14,13 +14,14 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.data.AccountAttribute; import com.google.gerrit.server.data.RefUpdateAttribute; public class RefUpdatedEvent extends RefEvent { - public AccountAttribute submitter; - public RefUpdateAttribute refUpdate; + public Supplier submitter; + public Supplier refUpdate; public RefUpdatedEvent() { super("ref-updated"); @@ -28,11 +29,11 @@ public class RefUpdatedEvent extends RefEvent { @Override public Project.NameKey getProjectNameKey() { - return new Project.NameKey(refUpdate.project); + return new Project.NameKey(refUpdate.get().project); } @Override public String getRefName() { - return refUpdate.refName; + return refUpdate.get().refName; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/ReviewerAddedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/ReviewerAddedEvent.java index b016bd9de5..483f68ee72 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/ReviewerAddedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/ReviewerAddedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class ReviewerAddedEvent extends PatchSetEvent { - public AccountAttribute reviewer; + public Supplier reviewer; public ReviewerAddedEvent() { super("reviewer-added"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/TopicChangedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/TopicChangedEvent.java index 7bb334fa7e..d038edd36a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/TopicChangedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/TopicChangedEvent.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.events; +import com.google.common.base.Supplier; import com.google.gerrit.server.data.AccountAttribute; public class TopicChangedEvent extends ChangeEvent { - public AccountAttribute changer; + public Supplier changer; public String oldTopic; public TopicChangedEvent() { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java index 379f1b92b9..91757585ef 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/StreamEvents.java @@ -17,6 +17,7 @@ package com.google.gerrit.sshd.commands; import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Supplier; import com.google.gerrit.common.EventListener; import com.google.gerrit.common.EventSource; import com.google.gerrit.common.data.GlobalCapability; @@ -30,12 +31,17 @@ import com.google.gerrit.sshd.BaseCommand; import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.StreamCommandExecutor; import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.JsonElement; +import com.google.gson.JsonSerializationContext; +import com.google.gson.JsonSerializer; import com.google.inject.Inject; import org.apache.sshd.server.Environment; import java.io.IOException; import java.io.PrintWriter; +import java.lang.reflect.Type; import java.util.concurrent.Future; import java.util.concurrent.LinkedBlockingQueue; @@ -63,7 +69,7 @@ final class StreamEvents extends BaseCommand { private final LinkedBlockingQueue queue = new LinkedBlockingQueue<>(MAX_EVENTS); - private final Gson gson = new Gson(); + private Gson gson; /** Special event to notify clients they missed other events. */ private static final class DroppedOutputEvent extends Event { @@ -139,6 +145,10 @@ final class StreamEvents extends BaseCommand { stdout = toPrintWriter(out); source.addEventListener(listener, currentUser); + + gson = new GsonBuilder() + .registerTypeAdapter(Supplier.class, new SupplierSerializer()) + .create(); } @Override @@ -247,4 +257,13 @@ final class StreamEvents extends BaseCommand { stdout.flush(); } } + + private static class SupplierSerializer + implements JsonSerializer> { + @Override + public JsonElement serialize(Supplier src, Type typeOfSrc, + JsonSerializationContext context) { + return context.serialize(src.get()); + } + } }