Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-69442

Gerrit server is queried for file changes even if the change event isn't of interest

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major Major
    • gerrit-trigger-plugin
    • None

      Prior to the code changes introduced in PR#425, the GerritTrigger.isInteresting() function included the following code to determine if the received change based event was of interest:

                          if (isServerInteresting(event)
                         && p.isInteresting(changeBasedEvent.getChange().getProject(),
                                            changeBasedEvent.getChange().getBranch(),
                                            changeBasedEvent.getChange().getTopic())) {

                        boolean containsFilePathsOrForbiddenFilePaths =
                                ((p.getFilePaths() != null && p.getFilePaths().size() > 0)
                                        || (p.getForbiddenFilePaths() != null && p.getForbiddenFilePaths().size() > 0));

                        if (isFileTriggerEnabled() && containsFilePathsOrForbiddenFilePaths) {
                            if (isServerInteresting(event)
                                 && p.isInteresting(changeBasedEvent.getChange().getProject(),
                                                    changeBasedEvent.getChange().getBranch(),
                                                    changeBasedEvent.getChange().getTopic(),
                                                    changeBasedEvent.getFiles(
                                                        new GerritQueryHandler(getServerConfig(event))))) {
                                logger.trace("According to {} the event is interesting; event: {}", p, event);
                                return true;
                            }
                        } else {
                            logger.trace("According to {} the event is interesting; event: {}", p, event);
                            return true;
                        }
                    }

      The important thing here is that the outer if block checks if the project, branch and topic match before the inner block gets the list of files for the change event. To do the later it has to query the Gerrit server since the information isn't present in the change event (hence the need to pass a GerritQueryHandler instance).

      However, after the changes were made (released in gerrit-trigger-2.33.0) the code was split out into a separate function, but the if blocks were also un-nested.

              if (isFileTriggerEnabled() && containsFilePathsOrForbiddenFilePaths) {
            if (project.isInteresting(change.getProject(), change.getBranch(), change.getTopic(),
                    change.getFiles(gerritQueryHandler))) {
                shouldTrigger = true;
            }
        } else {
            if (project.isInteresting(change.getProject(), change.getBranch(), change.getTopic())) {
                shouldTrigger = true;
            }
        }

      The effect of this is that the list of files is fetched before any test of the project, branch and topic. So for every job triggered by Gerrit change events, if there are any file path filters defined then Jenkins will always query Gerrit to get the list of file paths even if ultimately the event will be discarded.

      In a large Gerrit/Jenkins environment, this very quickly results in a large number of unnecessary requests. Also, since GerritTrigger.isInteresting() is called from GerritTrigger.gerritEvent(), the incoming event queue worker pool threads end up blocked waiting on the file path queries to Gerrit, which in turn causes a backlog in the incoming event handler queue to build up that is also visible in the logs. For example:

      The Gerrit incoming events queue contains 12279 items! Something might be stuck, or your system can't process the commands fast enough.

            rsandell rsandell
            chrisa_arm Chris
            Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: