SONAR-11455 improve SQL and network perf of PersistIssuesStep

server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/issue/UpdateConflictResolver.java

@@ -34,14 +34,10 @@ public class UpdateConflictResolver {
 
   private static final Logger LOG = Loggers.get(UpdateConflictResolver.class);
 
-  public void resolve(DefaultIssue issue, IssueMapper mapper) {
+  public void resolve(DefaultIssue issue, IssueDto dbIssue, IssueMapper mapper) {
     LOG.debug("Resolve conflict on issue {}", issue.key());
-
-    IssueDto dbIssue = mapper.selectByKey(issue.key());
-    if (dbIssue != null) {
-      mergeFields(dbIssue, issue);
-      mapper.update(IssueDto.toDtoForUpdate(issue, System.currentTimeMillis()));
-    }
+    mergeFields(dbIssue, issue);
+    mapper.update(IssueDto.toDtoForUpdate(issue, System.currentTimeMillis()));
   }
 
   @VisibleForTesting

server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/step/PersistIssuesStep.java

@@ -19,6 +19,9 @@
  */
 package org.sonar.ce.task.projectanalysis.step;
 
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
 import org.sonar.api.utils.System2;
 import org.sonar.ce.task.projectanalysis.issue.IssueCache;
 import org.sonar.ce.task.projectanalysis.issue.RuleRepository;
@@ -26,6 +29,7 @@ import org.sonar.ce.task.projectanalysis.issue.UpdateConflictResolver;
 import org.sonar.ce.task.step.ComputationStep;
 import org.sonar.core.issue.DefaultIssue;
 import org.sonar.core.util.CloseableIterator;
+import org.sonar.db.BatchSession;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.issue.IssueChangeMapper;
@@ -33,7 +37,13 @@ import org.sonar.db.issue.IssueDto;
 import org.sonar.db.issue.IssueMapper;
 import org.sonar.server.issue.IssueStorage;
 
+import static org.sonar.core.util.stream.MoreCollectors.toList;
+import static org.sonar.core.util.stream.MoreCollectors.uniqueIndex;
+
 public class PersistIssuesStep implements ComputationStep {
+  // holding up to 1000 DefaultIssue (max size of addedIssues and updatedIssues at any given time) in memory should not
+  // be a problem while making sure we leverage extensively the batch feature to speed up persistence
+  private static final int ISSUE_BATCHING_SIZE = BatchSession.MAX_BATCH_SIZE * 2;
 
   private final DbClient dbClient;
   private final System2 system2;
@@ -57,54 +67,84 @@ public class PersistIssuesStep implements ComputationStep {
     IssueStatistics statistics = new IssueStatistics();
     try (DbSession dbSession = dbClient.openSession(true);
       CloseableIterator<DefaultIssue> issues = issueCache.traverse()) {
+      List<DefaultIssue> addedIssues = new ArrayList<>(ISSUE_BATCHING_SIZE);
+      List<DefaultIssue> updatedIssues = new ArrayList<>(ISSUE_BATCHING_SIZE);
 
       IssueMapper mapper = dbSession.getMapper(IssueMapper.class);
       IssueChangeMapper changeMapper = dbSession.getMapper(IssueChangeMapper.class);
       while (issues.hasNext()) {
         DefaultIssue issue = issues.next();
-        boolean saved = persistIssueIfRequired(mapper, issue, statistics);
-        if (saved) {
-          issueStorage.insertChanges(changeMapper, issue);
+        if (issue.isNew() || issue.isCopied()) {
+          addedIssues.add(issue);
+          if (addedIssues.size() >= ISSUE_BATCHING_SIZE) {
+            persistNewIssues(statistics, addedIssues, mapper, changeMapper);
+            addedIssues.clear();
+          }
+        } else if (issue.isChanged()) {
+          updatedIssues.add(issue);
+          if (updatedIssues.size() >= ISSUE_BATCHING_SIZE) {
+            persistUpdatedIssues(statistics, updatedIssues, mapper, changeMapper);
+            updatedIssues.clear();
+          }
+        } else {
+          statistics.untouched++;
         }
       }
-      dbSession.flushStatements();
-      dbSession.commit();
+      persistNewIssues(statistics, addedIssues, mapper, changeMapper);
+      persistUpdatedIssues(statistics, updatedIssues, mapper, changeMapper);
+      flushSession(dbSession);
     } finally {
       statistics.dumpTo(context);
     }
   }
 
-  private boolean persistIssueIfRequired(IssueMapper mapper, DefaultIssue issue, IssueStatistics issueStatistics) {
-    if (issue.isNew() || issue.isCopied()) {
-      persistNewIssue(mapper, issue);
-      issueStatistics.inserts++;
-      return true;
+  private void persistNewIssues(IssueStatistics statistics, List<DefaultIssue> addedIssues, IssueMapper mapper, IssueChangeMapper changeMapper) {
+    if (addedIssues.isEmpty()) {
+      return;
     }
 
-    if (issue.isChanged()) {
-      persistChangedIssue(mapper, issue);
-      issueStatistics.updates++;
-      return true;
-    }
+    long now = system2.now();
+    addedIssues.forEach(i -> {
+      int ruleId = ruleRepository.getByKey(i.ruleKey()).getId();
+      IssueDto dto = IssueDto.toDtoForComputationInsert(i, ruleId, now);
+      mapper.insert(dto);
+      statistics.inserts++;
+    });
 
-    issueStatistics.untouched++;
-    return false;
+    addedIssues.forEach(i -> issueStorage.insertChanges(changeMapper, i));
   }
 
-  private void persistNewIssue(IssueMapper mapper, DefaultIssue issue) {
-    int ruleId = ruleRepository.getByKey(issue.ruleKey()).getId();
-    IssueDto dto = IssueDto.toDtoForComputationInsert(issue, ruleId, system2.now());
-    mapper.insert(dto);
-  }
+  private void persistUpdatedIssues(IssueStatistics statistics, List<DefaultIssue> updatedIssues, IssueMapper mapper, IssueChangeMapper changeMapper) {
+    if (updatedIssues.isEmpty()) {
+      return;
+    }
 
-  private void persistChangedIssue(IssueMapper mapper, DefaultIssue issue) {
-    IssueDto dto = IssueDto.toDtoForUpdate(issue, system2.now());
-    int updateCount = mapper.updateIfBeforeSelectedDate(dto);
-    if (updateCount == 0) {
-      // End-user and scan changed the issue at the same time.
-      // See https://jira.sonarsource.com/browse/SONAR-4309
-      conflictResolver.resolve(issue, mapper);
+    long now = system2.now();
+    updatedIssues.forEach(i -> {
+      IssueDto dto = IssueDto.toDtoForUpdate(i, now);
+      mapper.updateIfBeforeSelectedDate(dto);
+      statistics.updates++;
+    });
+
+    // retrieve those of the updatedIssues which have not been updated and apply conflictResolver on them
+    List<String> updatedIssueKeys = updatedIssues.stream().map(DefaultIssue::key).collect(toList(updatedIssues.size()));
+    List<IssueDto> conflictIssueKeys = mapper.selectByKeysIfNotUpdatedAt(updatedIssueKeys, now);
+    if (!conflictIssueKeys.isEmpty()) {
+      Map<String, DefaultIssue> issuesByKeys = updatedIssues.stream().collect(uniqueIndex(DefaultIssue::key, updatedIssues.size()));
+      conflictIssueKeys
+        .forEach(dbIssue -> {
+          DefaultIssue updatedIssue = issuesByKeys.get(dbIssue.getKey());
+          conflictResolver.resolve(updatedIssue, dbIssue, mapper);
+          statistics.merged++;
+        });
     }
+
+    updatedIssues.forEach(i -> issueStorage.insertChanges(changeMapper, i));
+  }
+
+  private static void flushSession(DbSession dbSession) {
+    dbSession.flushStatements();
+    dbSession.commit();
   }
 
   @Override
@@ -115,12 +155,14 @@ public class PersistIssuesStep implements ComputationStep {
   private static class IssueStatistics {
     private int inserts = 0;
     private int updates = 0;
+    private int merged = 0;
     private int untouched = 0;
 
     private void dumpTo(ComputationStep.Context context) {
       context.getStatistics()
         .add("inserts", String.valueOf(inserts))
         .add("updates", String.valueOf(updates))
+        .add("merged", String.valueOf(merged))
         .add("untouched", String.valueOf(untouched));
     }
   }

server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/issue/UpdateConflictResolverTest.java

@@ -32,7 +32,6 @@ import org.sonar.db.issue.IssueMapper;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 import static org.sonar.api.issue.Issue.STATUS_OPEN;
 import static org.sonar.api.rules.RuleType.CODE_SMELL;
 
@@ -51,21 +50,20 @@ public class UpdateConflictResolverTest {
 
     // Issue as seen and changed by end-user
     IssueMapper mapper = mock(IssueMapper.class);
-    when(mapper.selectByKey("ABCDE")).thenReturn(
-      new IssueDto()
-        .setKee("ABCDE")
-        .setType(CODE_SMELL)
-        .setRuleId(10)
-        .setRuleKey("squid", "AvoidCycles")
-        .setProjectUuid("U1")
-        .setComponentUuid("U2")
-        .setLine(10)
-        .setStatus(STATUS_OPEN)
-
-        // field changed by user
-        .setAssigneeUuid("arthur-uuid"));
-
-    new UpdateConflictResolver().resolve(issue, mapper);
+    IssueDto issueDto = new IssueDto()
+      .setKee("ABCDE")
+      .setType(CODE_SMELL)
+      .setRuleId(10)
+      .setRuleKey("squid", "AvoidCycles")
+      .setProjectUuid("U1")
+      .setComponentUuid("U2")
+      .setLine(10)
+      .setStatus(STATUS_OPEN)
+
+      // field changed by user
+      .setAssigneeUuid("arthur-uuid");
+
+    new UpdateConflictResolver().resolve(issue, issueDto, mapper);
 
     ArgumentCaptor<IssueDto> argument = ArgumentCaptor.forClass(IssueDto.class);
     verify(mapper).update(argument.capture());

server/sonar-ce-task-projectanalysis/src/test/java/org/sonar/ce/task/projectanalysis/step/PersistIssuesStepTest.java

@@ -27,6 +27,7 @@ import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
+import org.mockito.ArgumentCaptor;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rules.RuleType;
 import org.sonar.api.utils.System2;
@@ -46,9 +47,9 @@ import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.DbTester;
 import org.sonar.db.component.ComponentDto;
-import org.sonar.db.component.ComponentTesting;
 import org.sonar.db.issue.IssueChangeDto;
 import org.sonar.db.issue.IssueDto;
+import org.sonar.db.issue.IssueMapper;
 import org.sonar.db.organization.OrganizationDto;
 import org.sonar.db.rule.RuleDefinitionDto;
 import org.sonar.db.rule.RuleTesting;
@@ -58,7 +59,10 @@ import org.sonar.server.issue.IssueStorage;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.data.MapEntry.entry;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.sonar.api.issue.Issue.RESOLUTION_FIXED;
 import static org.sonar.api.issue.Issue.STATUS_CLOSED;
@@ -80,8 +84,10 @@ public class PersistIssuesStepTest extends BaseStepTest {
   public AnalysisMetadataHolderRule analysisMetadataHolder = new AnalysisMetadataHolderRule()
     .setOrganizationUuid("org-1", "qg-uuid-1");
 
+  private System2 system2 = mock(System2.class);
   private DbSession session = db.getSession();
   private DbClient dbClient = db.getDbClient();
+  private UpdateConflictResolver conflictResolver = mock(UpdateConflictResolver.class);
   private IssueCache issueCache;
   private ComputationStep underTest;
 
@@ -95,11 +101,9 @@ public class PersistIssuesStepTest extends BaseStepTest {
   @Before
   public void setup() throws Exception {
     issueCache = new IssueCache(temp.newFile(), System2.INSTANCE);
-    System2 system2 = mock(System2.class);
-    when(system2.now()).thenReturn(NOW);
     reportReader.setMetadata(ScannerReport.Metadata.getDefaultInstance());
 
-    underTest = new PersistIssuesStep(dbClient, system2, new UpdateConflictResolver(), new RuleRepositoryImpl(adHocRuleCreator, dbClient, analysisMetadataHolder), issueCache,
+    underTest = new PersistIssuesStep(dbClient, system2, conflictResolver, new RuleRepositoryImpl(adHocRuleCreator, dbClient, analysisMetadataHolder), issueCache,
       new IssueStorage());
   }
 
@@ -113,11 +117,9 @@ public class PersistIssuesStepTest extends BaseStepTest {
     RuleDefinitionDto rule = RuleTesting.newRule(RuleKey.of("xoo", "S01"));
     db.rules().insert(rule);
     OrganizationDto organizationDto = db.organizations().insert();
-    ComponentDto project = ComponentTesting.newPrivateProjectDto(organizationDto);
-    dbClient.componentDao().insert(session, project);
-    ComponentDto file = newFileDto(project, null);
-    dbClient.componentDao().insert(session, file);
-    session.commit();
+    ComponentDto project = db.components().insertPrivateProject(organizationDto);
+    ComponentDto file = db.components().insertComponent(newFileDto(project, null));
+    when(system2.now()).thenReturn(NOW);
 
     issueCache.newAppender().append(new DefaultIssue()
       .setKey("ISSUE")
@@ -130,6 +132,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
       .setNew(false)
       .setCopied(true)
       .setType(RuleType.BUG)
+      .setSelectedAt(NOW)
       .addComment(new DefaultIssueComment()
         .setKey("COMMENT")
         .setIssueKey("ISSUE")
@@ -160,7 +163,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
     List<IssueChangeDto> changes = dbClient.issueChangeDao().selectByIssueKeys(session, Arrays.asList("ISSUE"));
     assertThat(changes).extracting(IssueChangeDto::getChangeType).containsExactly(IssueChangeDto.TYPE_COMMENT, IssueChangeDto.TYPE_FIELD_CHANGE);
     assertThat(context.getStatistics().getAll()).containsOnly(
-      entry("inserts", "1"), entry("updates", "0"), entry("untouched", "0"));
+      entry("inserts", "1"), entry("updates", "0"), entry("merged", "0"), entry("untouched", "0"));
   }
 
   @Test
@@ -168,11 +171,9 @@ public class PersistIssuesStepTest extends BaseStepTest {
     RuleDefinitionDto rule = RuleTesting.newRule(RuleKey.of("xoo", "S01"));
     db.rules().insert(rule);
     OrganizationDto organizationDto = db.organizations().insert();
-    ComponentDto project = ComponentTesting.newPrivateProjectDto(organizationDto);
-    dbClient.componentDao().insert(session, project);
-    ComponentDto file = newFileDto(project, null);
-    dbClient.componentDao().insert(session, file);
-    session.commit();
+    ComponentDto project = db.components().insertPrivateProject(organizationDto);
+    ComponentDto file = db.components().insertComponent(newFileDto(project, null));
+    when(system2.now()).thenReturn(NOW);
 
     issueCache.newAppender().append(new DefaultIssue()
       .setKey("ISSUE")
@@ -185,6 +186,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
       .setNew(true)
       .setCopied(true)
       .setType(RuleType.BUG)
+      .setSelectedAt(NOW)
       .addComment(new DefaultIssueComment()
         .setKey("COMMENT")
         .setIssueKey("ISSUE")
@@ -214,7 +216,43 @@ public class PersistIssuesStepTest extends BaseStepTest {
     List<IssueChangeDto> changes = dbClient.issueChangeDao().selectByIssueKeys(session, Arrays.asList("ISSUE"));
     assertThat(changes).extracting(IssueChangeDto::getChangeType).containsExactly(IssueChangeDto.TYPE_COMMENT, IssueChangeDto.TYPE_FIELD_CHANGE);
     assertThat(context.getStatistics().getAll()).containsOnly(
-      entry("inserts", "1"), entry("updates", "0"), entry("untouched", "0"));
+      entry("inserts", "1"), entry("updates", "0"), entry("merged", "0"), entry("untouched", "0"));
+  }
+
+  @Test
+  public void update_conflicting_issue() {
+    RuleDefinitionDto rule = RuleTesting.newRule(RuleKey.of("xoo", "S01"));
+    db.rules().insert(rule);
+    OrganizationDto organizationDto = db.organizations().insert();
+    ComponentDto project = db.components().insertPrivateProject(organizationDto);
+    ComponentDto file = db.components().insertComponent(newFileDto(project, null));
+    IssueDto issue = db.issues().insert(rule, project, file,
+      i -> i.setStatus(STATUS_OPEN)
+        .setResolution(null)
+        .setCreatedAt(NOW - 1_000_000_000L)
+        // simulate the issue has been updated after the analysis ran
+        .setUpdatedAt(NOW + 1_000_000_000L));
+    issue = dbClient.issueDao().selectByKey(db.getSession(), issue.getKey()).get();
+    DiskCache<DefaultIssue>.DiskAppender issueCacheAppender = issueCache.newAppender();
+    when(system2.now()).thenReturn(NOW);
+
+    DefaultIssue defaultIssue = issue.toDefaultIssue()
+      .setStatus(STATUS_CLOSED)
+      .setResolution(RESOLUTION_FIXED)
+      .setSelectedAt(NOW)
+      .setNew(false)
+      .setChanged(true);
+    issueCacheAppender.append(defaultIssue).close();
+
+    TestComputationStepContext context = new TestComputationStepContext();
+    underTest.execute(context);
+
+    ArgumentCaptor<IssueDto> issueDtoCaptor = ArgumentCaptor.forClass(IssueDto.class);
+    verify(conflictResolver).resolve(eq(defaultIssue), issueDtoCaptor.capture(), any(IssueMapper.class));
+    assertThat(issueDtoCaptor.getValue().getId()).isEqualTo(issue.getId());
+    assertThat(context.getStatistics().getAll()).containsOnly(
+      entry("inserts", "0"), entry("updates", "1"), entry("merged", "1"), entry("untouched", "0"));
+
   }
 
   @Test
@@ -222,10 +260,8 @@ public class PersistIssuesStepTest extends BaseStepTest {
     RuleDefinitionDto rule = RuleTesting.newRule(RuleKey.of("xoo", "S01"));
     db.rules().insert(rule);
     OrganizationDto organizationDto = db.organizations().insert();
-    ComponentDto project = ComponentTesting.newPrivateProjectDto(organizationDto);
-    dbClient.componentDao().insert(session, project);
-    ComponentDto file = newFileDto(project, null);
-    dbClient.componentDao().insert(session, file);
+    ComponentDto project = db.components().insertPrivateProject(organizationDto);
+    ComponentDto file = db.components().insertComponent(newFileDto(project, null));
     session.commit();
 
     issueCache.newAppender().append(new DefaultIssue()
@@ -251,7 +287,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
     assertThat(result.getStatus()).isEqualTo(STATUS_OPEN);
     assertThat(result.getType()).isEqualTo(RuleType.BUG.getDbConstant());
     assertThat(context.getStatistics().getAll()).containsOnly(
-      entry("inserts", "1"), entry("updates", "0"), entry("untouched", "0"));
+      entry("inserts", "1"), entry("updates", "0"), entry("merged", "0"), entry("untouched", "0"));
   }
 
   @Test
@@ -282,7 +318,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
     assertThat(issueReloaded.getStatus()).isEqualTo(STATUS_CLOSED);
     assertThat(issueReloaded.getResolution()).isEqualTo(RESOLUTION_FIXED);
     assertThat(context.getStatistics().getAll()).containsOnly(
-      entry("inserts", "0"), entry("updates", "1"), entry("untouched", "0"));
+      entry("inserts", "0"), entry("updates", "1"), entry("merged", "0"), entry("untouched", "0"));
   }
 
   @Test
@@ -322,7 +358,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
         IssueChangeDto::getIssueChangeCreationDate)
       .containsOnly(IssueChangeDto.TYPE_COMMENT, "john_uuid", "Some text", issue.getKey(), NOW);
     assertThat(context.getStatistics().getAll()).containsOnly(
-      entry("inserts", "0"), entry("updates", "1"), entry("untouched", "0"));
+      entry("inserts", "0"), entry("updates", "1"), entry("merged", "0"), entry("untouched", "0"));
   }
 
   @Test
@@ -360,7 +396,7 @@ public class PersistIssuesStepTest extends BaseStepTest {
         IssueChangeDto::getIssueChangeCreationDate)
       .containsOnly(IssueChangeDto.TYPE_FIELD_CHANGE, "john_uuid", "technicalDebt=1", issue.getKey(), NOW);
     assertThat(context.getStatistics().getAll()).containsOnly(
-      entry("inserts", "0"), entry("updates", "1"), entry("untouched", "0"));
+      entry("inserts", "0"), entry("updates", "1"), entry("merged", "0"), entry("untouched", "0"));
   }
 
 }

server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueMapper.java

@@ -34,6 +34,8 @@ public interface IssueMapper {
 
   List<IssueDto> selectByKeys(List<String> keys);
 
+  List<IssueDto> selectByKeysIfNotUpdatedAt(@Param("keys") List<String> keys, @Param("updatedAt") long updatedAt);
+
   List<ShortBranchIssueDto> selectOpenByComponentUuids(List<String> componentUuids);
 
   void insert(IssueDto issue);
@@ -53,4 +55,5 @@ public interface IssueMapper {
   Collection<IssueGroupDto> selectIssueGroupsByBaseComponent(
     @Param("baseComponent") ComponentDto baseComponent,
     @Param("leakPeriodBeginningDate") long leakPeriodBeginningDate);
+
 }

server/sonar-db-dao/src/main/resources/org/sonar/db/issue/IssueMapper.xml

@@ -261,6 +261,21 @@
     </foreach>
   </select>
 
+  <select id="selectByKeysIfNotUpdatedAt" parameterType="map" resultType="Issue">
+    select
+      <include refid="issueColumns"/>
+    from issues i
+    inner join rules r on r.id=i.rule_id
+    inner join projects p on p.uuid=i.component_uuid
+    inner join projects root on root.uuid=i.project_uuid
+    where
+      i.kee in
+        <foreach collection="keys" open="(" close=")" item="key" separator=",">
+          #{key,jdbcType=VARCHAR}
+        </foreach>
+      and i.updated_at &lt;&gt; #{updatedAt,jdbcType=BIGINT}
+  </select>
+
   <select id="selectOpenByComponentUuids" parameterType="map" resultType="ShortBranchIssue">
     select
       i.kee as kee,