From 08a18e07cc842e111f834a395da414fd516606ee Mon Sep 17 00:00:00 2001 From: Julien HENRY Date: Tue, 27 Aug 2013 10:30:55 +0200 Subject: [PATCH] SONAR-4524 Create notifications using batch insert to improve performances --- .../SendIssueNotificationsPostJob.java | 9 +++- .../SendIssueNotificationsPostJobTest.java | 43 ++++++++++++++++--- .../sonar/core/issue/IssueNotifications.java | 27 ++++++++---- .../DefaultNotificationManager.java | 13 +++++- .../notification/db/NotificationQueueDao.java | 8 ++-- .../notification/db/NotificationQueueDto.java | 35 ++------------- .../sonar/core/persistence/BatchSession.java | 2 +- .../db/NotificationQueueMapper.xml | 8 ++-- .../core/issue/IssueNotificationsTest.java | 27 ++++++------ .../DefaultNotificationManagerTest.java | 2 +- .../db/NotificationQueueDaoTest.java | 2 +- .../notifications/NotificationManager.java | 20 ++++++--- 12 files changed, 119 insertions(+), 77 deletions(-) diff --git a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java index 0a3658a34a0..0cbe7456c6e 100644 --- a/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java +++ b/plugins/sonar-core-plugin/src/main/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJob.java @@ -30,6 +30,9 @@ import org.sonar.batch.issue.IssueCache; import org.sonar.core.DryRunIncompatible; import org.sonar.core.issue.IssueNotifications; +import java.util.LinkedHashMap; +import java.util.Map; + /** * @since 3.6 */ @@ -54,6 +57,7 @@ public class SendIssueNotificationsPostJob implements PostJob { private void sendNotifications(Project project) { int newIssues = 0; IssueChangeContext context = IssueChangeContext.createScan(project.getAnalysisDate()); + Map shouldSentNotification = new LinkedHashMap(); for (DefaultIssue issue : issueCache.all()) { if (issue.isNew() && issue.resolution() == null) { newIssues++; @@ -62,10 +66,13 @@ public class SendIssueNotificationsPostJob implements PostJob { Rule rule = ruleFinder.findByKey(issue.ruleKey()); // TODO warning - rules with status REMOVED are currently ignored, but should not if (rule != null) { - notifications.sendChanges(issue, context, rule, project, null); + shouldSentNotification.put(issue, rule); } } } + if (!shouldSentNotification.isEmpty()) { + notifications.sendChanges(shouldSentNotification, context, project, null); + } if (newIssues > 0) { notifications.sendNewIssues(project, newIssues); } diff --git a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java index 736049d7ae8..82a1fe70ff4 100644 --- a/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java +++ b/plugins/sonar-core-plugin/src/test/java/org/sonar/plugins/core/issue/notification/SendIssueNotificationsPostJobTest.java @@ -21,6 +21,7 @@ package org.sonar.plugins.core.issue.notification; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentMatcher; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.sonar.api.batch.SensorContext; @@ -36,8 +37,17 @@ import org.sonar.batch.issue.IssueCache; import org.sonar.core.issue.IssueNotifications; import java.util.Arrays; - -import static org.mockito.Mockito.*; +import java.util.Map; + +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.argThat; +import static org.mockito.Matchers.eq; +import static org.mockito.Matchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; @RunWith(MockitoJUnitRunner.class) public class SendIssueNotificationsPostJobTest { @@ -62,7 +72,7 @@ public class SendIssueNotificationsPostJobTest { when(issueCache.all()).thenReturn(Arrays.asList( new DefaultIssue().setNew(true), new DefaultIssue().setNew(false) - )); + )); SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder); job.executeOn(project, sensorContext); @@ -75,7 +85,7 @@ public class SendIssueNotificationsPostJobTest { when(project.getAnalysisDate()).thenReturn(DateUtils.parseDate("2013-05-18")); when(issueCache.all()).thenReturn(Arrays.asList( new DefaultIssue().setNew(false) - )); + )); SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder); job.executeOn(project, sensorContext); @@ -100,7 +110,7 @@ public class SendIssueNotificationsPostJobTest { SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder); job.executeOn(project, sensorContext); - verify(notifications).sendChanges(eq(issue), any(IssueChangeContext.class), eq(rule), any(Component.class), (Component)isNull()); + verify(notifications).sendChanges(argThat(matchMapOf(issue, rule)), any(IssueChangeContext.class), any(Component.class), (Component) isNull()); } @Test @@ -119,7 +129,7 @@ public class SendIssueNotificationsPostJobTest { SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder); job.executeOn(project, sensorContext); - verify(notifications, never()).sendChanges(eq(issue), eq(changeContext), any(Rule.class), any(Component.class), any(Component.class)); + verify(notifications, never()).sendChanges(argThat(matchMapOf(issue, null)), eq(changeContext), any(Component.class), any(Component.class)); } @Test @@ -139,6 +149,25 @@ public class SendIssueNotificationsPostJobTest { SendIssueNotificationsPostJob job = new SendIssueNotificationsPostJob(issueCache, notifications, ruleFinder); job.executeOn(project, sensorContext); - verify(notifications, never()).sendChanges(eq(issue), eq(changeContext), any(Rule.class), any(Component.class), any(Component.class)); + verify(notifications, never()).sendChanges(argThat(matchMapOf(issue, null)), eq(changeContext), any(Component.class), any(Component.class)); + } + + private static IsMapOfIssueAndRule matchMapOf(DefaultIssue issue, Rule rule) { + return new IsMapOfIssueAndRule(issue, rule); + } + + static class IsMapOfIssueAndRule extends ArgumentMatcher> { + private DefaultIssue issue; + private Rule rule; + + public IsMapOfIssueAndRule(DefaultIssue issue, Rule rule) { + this.issue = issue; + this.rule = rule; + } + + public boolean matches(Object arg) { + Map map = (Map) arg; + return map.size() == 1 && map.get(issue) != null && map.get(issue).equals(rule); + } } } diff --git a/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java b/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java index 1a96d48446d..5619105e218 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/IssueNotifications.java @@ -19,6 +19,8 @@ */ package org.sonar.core.issue; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import org.sonar.api.BatchComponent; import org.sonar.api.ServerComponent; import org.sonar.api.component.Component; @@ -35,8 +37,11 @@ import org.sonar.core.i18n.RuleI18nManager; import javax.annotation.CheckForNull; import javax.annotation.Nullable; + +import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Map.Entry; /** * Send notifications related to issues. @@ -63,17 +68,23 @@ public class IssueNotifications implements BatchComponent, ServerComponent { } @CheckForNull - public Notification sendChanges(DefaultIssue issue, IssueChangeContext context, IssueQueryResult queryResult) { - return sendChanges(issue, context, queryResult.rule(issue), queryResult.project(issue), queryResult.component(issue)); + public List sendChanges(DefaultIssue issue, IssueChangeContext context, IssueQueryResult queryResult) { + Map issues = Maps.newHashMap(); + issues.put(issue, queryResult.rule(issue)); + return sendChanges(issues, context, queryResult.project(issue), queryResult.component(issue)); } @CheckForNull - public Notification sendChanges(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, @Nullable Component component) { - Notification notification = createChangeNotification(issue, context, rule, project, component, null); - if (notification != null) { - notificationsManager.scheduleForSending(notification); + public List sendChanges(Map issues, IssueChangeContext context, Component project, @Nullable Component component) { + List notifications = Lists.newArrayList(); + for (Entry entry : issues.entrySet()) { + Notification notification = createChangeNotification(entry.getKey(), context, entry.getValue(), project, component, null); + if (notification != null) { + notifications.add(notification); + } } - return notification; + notificationsManager.scheduleForSending(notifications); + return notifications; } @CheckForNull @@ -87,7 +98,7 @@ public class IssueNotifications implements BatchComponent, ServerComponent { @CheckForNull private Notification createChangeNotification(DefaultIssue issue, IssueChangeContext context, Rule rule, Component project, - @Nullable Component component, @Nullable String comment) { + @Nullable Component component, @Nullable String comment) { Notification notification = null; if (comment != null || issue.mustSendNotifications()) { FieldDiffs currentChange = issue.currentChange(); diff --git a/sonar-core/src/main/java/org/sonar/core/notification/DefaultNotificationManager.java b/sonar-core/src/main/java/org/sonar/core/notification/DefaultNotificationManager.java index 6d8b17921a2..e57c182f566 100644 --- a/sonar-core/src/main/java/org/sonar/core/notification/DefaultNotificationManager.java +++ b/sonar-core/src/main/java/org/sonar/core/notification/DefaultNotificationManager.java @@ -20,7 +20,9 @@ package org.sonar.core.notification; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Function; import com.google.common.collect.HashMultimap; +import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; import org.sonar.api.notifications.Notification; @@ -66,7 +68,16 @@ public class DefaultNotificationManager implements NotificationManager { */ public void scheduleForSending(Notification notification) { NotificationQueueDto dto = NotificationQueueDto.toNotificationQueueDto(notification); - notificationQueueDao.insert(dto); + notificationQueueDao.insert(Arrays.asList(dto)); + } + + public void scheduleForSending(List notification) { + notificationQueueDao.insert(Lists.transform(notification, new Function() { + @Override + public NotificationQueueDto apply(Notification notification) { + return NotificationQueueDto.toNotificationQueueDto(notification); + } + })); } /** diff --git a/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDao.java b/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDao.java index 8bb11e157d8..1152e48937b 100644 --- a/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDao.java +++ b/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDao.java @@ -39,10 +39,12 @@ public class NotificationQueueDao implements BatchComponent, ServerComponent { this.mybatis = mybatis; } - public void insert(NotificationQueueDto dto) { - SqlSession session = mybatis.openSession(); + public void insert(List dtos) { + SqlSession session = mybatis.openBatchSession(); try { - session.getMapper(NotificationQueueMapper.class).insert(dto); + for (NotificationQueueDto dto : dtos) { + session.getMapper(NotificationQueueMapper.class).insert(dto); + } session.commit(); } finally { MyBatis.closeQuietly(session); diff --git a/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDto.java b/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDto.java index 4e9668a507b..a42edc1a86e 100644 --- a/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDto.java +++ b/sonar-core/src/main/java/org/sonar/core/notification/db/NotificationQueueDto.java @@ -31,7 +31,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; -import java.util.Date; /** * @since 4.0 @@ -39,7 +38,6 @@ import java.util.Date; public class NotificationQueueDto { private Long id; - private Date createdAt; private byte[] data; public Long getId() { @@ -51,15 +49,6 @@ public class NotificationQueueDto { return this; } - public Date getCreatedAt() { - return createdAt; - } - - public NotificationQueueDto setCreatedAt(Date createdAt) { - this.createdAt = createdAt; - return this; - } - public byte[] getData() { return data; } @@ -69,24 +58,6 @@ public class NotificationQueueDto { return this; } - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - - NotificationQueueDto actionPlanDto = (NotificationQueueDto) o; - return !(id != null ? !id.equals(actionPlanDto.id) : actionPlanDto.id != null); - } - - @Override - public int hashCode() { - return id != null ? id.hashCode() : 0; - } - @Override public String toString() { return ToStringBuilder.reflectionToString(this, ToStringStyle.SHORT_PREFIX_STYLE); @@ -101,7 +72,7 @@ public class NotificationQueueDto { return new NotificationQueueDto().setData(byteArrayOutputStream.toByteArray()); } catch (IOException e) { - throw new SonarException(e); + throw new SonarException("Unable to write notification", e); } finally { IOUtils.closeQuietly(byteArrayOutputStream); @@ -121,10 +92,10 @@ public class NotificationQueueDto { return (Notification) result; } catch (IOException e) { - throw new SonarException(e); + throw new SonarException("Unable to read notification", e); } catch (ClassNotFoundException e) { - throw new SonarException(e); + throw new SonarException("Unable to read notification", e); } finally { IOUtils.closeQuietly(byteArrayInputStream); diff --git a/sonar-core/src/main/java/org/sonar/core/persistence/BatchSession.java b/sonar-core/src/main/java/org/sonar/core/persistence/BatchSession.java index 739fbfdc974..d19383176c2 100644 --- a/sonar-core/src/main/java/org/sonar/core/persistence/BatchSession.java +++ b/sonar-core/src/main/java/org/sonar/core/persistence/BatchSession.java @@ -123,7 +123,7 @@ public final class BatchSession implements SqlSession { if (null != mappedStatement) { KeyGenerator keyGenerator = mappedStatement.getKeyGenerator(); if (keyGenerator instanceof Jdbc3KeyGenerator) { - throw new IllegalStateException("Batch updates cannot use generated keys"); + throw new IllegalStateException("Batch inserts cannot use generated keys"); } } } diff --git a/sonar-core/src/main/resources/org/sonar/core/notification/db/NotificationQueueMapper.xml b/sonar-core/src/main/resources/org/sonar/core/notification/db/NotificationQueueMapper.xml index c14850598b2..eb933957b73 100644 --- a/sonar-core/src/main/resources/org/sonar/core/notification/db/NotificationQueueMapper.xml +++ b/sonar-core/src/main/resources/org/sonar/core/notification/db/NotificationQueueMapper.xml @@ -4,7 +4,7 @@ - + INSERT INTO notifications (created_at, data) VALUES (current_timestamp, #{data}) @@ -14,7 +14,7 @@ - select top (#{count}) id, created_at, data + select top (#{count}) id, data from notifications order by created_at asc @@ -30,7 +30,7 @@