Merge branch 'dev' into chicago-rta-fares

Author: ansoncfit

.github/workflows/codeql-analysis.yml

@@ -43,6 +43,15 @@ jobs:
     - name: Checkout repository
       uses: actions/checkout@v3
 
+    # Java setup step completes very fast, no need to run in a preconfigured docker container.
+    # CodeQL is intended to detect any Java toolchains added to the execution environment.
+    - name: Set up JDK 21
+      uses: actions/setup-java@v3
+      with:
+        distribution: corretto
+        java-version: 21
+        cache: gradle
+
     # Initializes the CodeQL tools for scanning.
     - name: Initialize CodeQL
       uses: github/codeql-action/init@v2

.github/workflows/cypress-integration.yml

@@ -28,24 +28,25 @@ jobs:
     #     BUILD_TARGET:staging
     steps:
     # Starting in v2.2 checkout action fetches all tags when fetch-depth=0, for auto-versioning.
-    - uses: actions/checkout@v2.3.2
+    - uses: actions/checkout@v3
       with:
         fetch-depth: 0
     # Java setup step completes very fast, no need to run in a preconfigured docker container.
-    - name: Set up JDK 11
+    - name: Set up JDK 21
       uses: actions/setup-java@v3
       with:
-        java-version: 11
-        distribution: temurin
-        cache: 'gradle'
+        distribution: corretto
+        java-version: 21
+        cache: gradle
     - name: Show version string
       run: gradle -q printVersion | head -n1
-    - name: Build and Test
+    - name: Build and test
       run: gradle build
-    - name: Ensure shadow JAR is runnable as local backend
+    # Check that build product is able to start up as a backend server before handing it to end-to-end testing
+    - name: Ensure backend runnable
       run: |
         cp analysis.properties.template analysis.properties
-        gradle testShadowJarRunnable
+        gradle testRunnable -x test
     - name: Publish to GH Packages
       # Supply access token to build.gradle (used in publishing.repositories.maven.credentials)
       env:

.github/workflows/gradle.yml

@@ -1,6 +1,6 @@
 MIT License
 
-Copyright (c) 2020 Conveyal
+Copyright (c) 2020-2023 Conveyal
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal

LICENSE

@@ -1,6 +1,6 @@
 plugins {
     id 'java'
-    id 'com.github.johnrengelman.shadow' version '8.1.0'
+    id 'application'
     id 'maven-publish'
     id 'com.palantir.git-version' version '2.0.0'
 }
@@ -10,18 +10,19 @@ group = 'com.conveyal'
 version gitVersion()
 
 java {
-    sourceCompatibility = JavaVersion.VERSION_11
-    targetCompatibility = JavaVersion.VERSION_11
+    toolchain {
+        languageVersion.set(JavaLanguageVersion.of(21))
+    }
 }
 
 jar {
-    // For Java 11 Modules, specify a module name.
+    // For Java 11+ Modules, specify a module name.
     // Do not create module-info.java until all our dependencies specify a module name.
     // Main-Class BackendMain will start a local backend.
     // Build-Jdk-Spec mimics a Maven manifest entry that helps us automatically install the right JVM.
     // Implementation-X attributes are needed for ImageIO (used by Geotools) to initialize in some environments.
     manifest {
-        attributes 'Automatic-Module-Name': 'com.conveyal.analysis',
+        attributes 'Automatic-Module-Name': 'com.conveyal.r5',
             'Main-Class': 'com.conveyal.analysis.BackendMain',
             'Build-Jdk-Spec': targetCompatibility.getMajorVersion(),
             'Implementation-Title': 'Conveyal Analysis Backend',
@@ -30,18 +31,15 @@ jar {
     }
 }
 
-shadowJar {
-    mergeServiceFiles()
-}
-
-// Allow reflective access by Kryo to normally closed Java internals.
-// This is used for testing equality, but also for building automatic Kryo (de)serializers.
+// Allow reflective access by ObjectDiffer to normally closed Java internals. Used for round-trip testing serialization.
+// IntelliJ seems not to pass these JVM arguments when running tests from within the IDE, so the Kryo serialization
+// tests may only succeed under command line Gradle.
 test {
+    useJUnitPlatform()
     jvmArgs = ['--add-opens=java.base/java.io=ALL-UNNAMED',
                '--add-opens=java.base/java.time=ALL-UNNAMED',
                '--add-opens=java.base/java.time.zone=ALL-UNNAMED',
                '--add-opens=java.base/java.lang=ALL-UNNAMED']
-    useJUnitPlatform()
 }
 
 // `gradle publish` will upload both shadow and simple JAR to Github Packages
@@ -80,19 +78,25 @@ task copyDependencies(type: Copy) {
     into 'dependencies'
 }
 
+application {
+    applicationDefaultJvmArgs = ['-Xmx6G']
+    mainClass = 'com.conveyal.analysis.BackendMain'
+}
+
 // Run R5 as a local analysis backend with all dependencies on the classpath, without building a shadowJar.
 task runBackend (type: JavaExec) {
-   dependsOn(build)
-   classpath(sourceSets.main.runtimeClasspath)
-   mainClass = 'com.conveyal.analysis.BackendMain'
+    dependsOn(build)
+    maxHeapSize('7G')
+    classpath(sourceSets.main.runtimeClasspath)
+    mainClass = 'com.conveyal.analysis.BackendMain'
 }
 
-// Start up an analysis local backend from a shaded JAR and ask it to shut down immediately.
-// This is used to check in the automated build that the JAR is usable before we keep it.
+// Start up an analysis local backend and ask it to shut down immediately.
+// This is used to check in the automated build that the JAR is usable in end-to-end tests before we keep it.
 // Create a configuration properties file (by copying the template) before running this task.
-task testShadowJarRunnable(type: JavaExec) {
-    dependsOn(shadowJar)
-    classpath(shadowJar.archiveFile.get())
+task testRunnable(type: JavaExec) {
+    dependsOn(build)
+    classpath(sourceSets.main.runtimeClasspath)
     mainClass = 'com.conveyal.analysis.BackendMain'
     jvmArgs("-Dconveyal.immediate.shutdown=true")
 }
@@ -116,6 +120,12 @@ task createVersionProperties(dependsOn: processResources) {
     }
 }
 
+// Fix inconsistent Gradle behavior (see https://github.com/gradle/gradle/issues/16791)
+// By default JavaExec tasks use the JVM Gradle was launched with, ignoring the project-level toolchain.
+tasks.withType(JavaExec).configureEach {
+    javaLauncher.set(javaToolchains.launcherFor(java.toolchain))
+}
+
 classes {
     dependsOn createVersionProperties
 }
@@ -136,10 +146,10 @@ configurations.all {
 
 dependencies {
     // Provides our logging API
-    implementation 'org.slf4j:slf4j-api:1.7.30'
+    implementation 'org.slf4j:slf4j-api:2.0.7'
 
     // Implementation of the logging API
-    implementation 'ch.qos.logback:logback-classic:1.2.3'
+    implementation 'ch.qos.logback:logback-classic:1.4.11'
 
     // Spark is an HTTP framework built on Jetty. Its name is the same as several other projects.
     implementation (group: 'com.sparkjava', name: 'spark-core', version: '2.7.2') {
@@ -195,9 +205,6 @@ dependencies {
     // Commons IO gives us BOMInputStream for handling UTF-8 Byte Order Marks.
     implementation 'commons-io:commons-io:2.6'
 
-    // Guava provides a lot of functionality, collections, and tools "missing" from the Java standard library.
-    implementation 'com.google.guava:guava:28.2-jre'
-
     // Java 8 rewrite of the Guava cache with asynchronous LoadingCaches. We don't currently use the async
     // capabilities, but Caffeine's LoadingCache syntax is more modern idiomatic Java than Guava's.
     implementation 'com.github.ben-manes.caffeine:caffeine:2.8.1'
@@ -214,15 +221,19 @@ dependencies {
     // Commons Math gives us FastMath, MersenneTwister, and low-discrepancy vector generators.
     implementation 'org.apache.commons:commons-math3:3.0'
 
-    // Provides some shared serializers for Kryo. Introduces transitive dependencies on Guava, Trove, and Kryo.
+    // Provides some Kryo serializers for Guava and Trove collecitons.
     // Also provides classes for testing that a round trip through serialization reproduces the same network.
     // This is an external dependency (not merged into backend) because it's also used by OTP2.
-    // TODO arguably we should declare non-transitive dependencies on Guava, Trove, and Kryo since we use them directly
-    implementation 'com.conveyal:kryo-tools:1.3.0'
+    implementation 'com.conveyal:kryo-tools:1.6.0'
 
+    // Ensure the versions of the next three dependencies match the transitive dependencies of kryo-tools.
+    implementation 'com.esotericsoftware:kryo:5.5.0'
+    // Guava provides a lot of functionality, collections, and tools "missing" from the Java standard library.
+    implementation 'com.google.guava:guava:32.1.2-jre'
     // Trove supplies very efficient collections of primitive data types for Java.
     implementation 'net.sf.trove4j:trove4j:3.0.3'
 
+
     // TODO eliminate custom Conveyal geojson library, use Geotools?
     implementation 'com.conveyal:jackson2-geojson:0.9'
 
@@ -245,8 +256,7 @@ dependencies {
     ////// Test-only dependencies //////
 
     // Java unit testing framework.
-    testImplementation(platform('org.junit:junit-bom:5.7.0'))
-    testImplementation('org.junit.jupiter:junit-jupiter')
+    testImplementation('org.junit.jupiter:junit-jupiter:5.9.2')
 
     // Chart drawing library for examining travel time distributions when crafting tests.
     // Although rarely used it should be low-impact: it is a test-only dependency with no transitive dependenices.

build.gradle

@@ -48,36 +48,41 @@
 /**
  * This class distributes the tasks making up regional jobs to workers.
  * <p>
- * It should aim to draw tasks fairly from all organizations, and fairly from all jobs within each
- * organization, while attempting to respect the transport network affinity of each worker, giving
- * the worker tasks that require the same network it has been using recently.
+ * It respects the declared transport network affinity of each worker, giving the worker tasks that
+ * relate to the same network it has been using recently, and is therefore already loaded in memory.
  * <p>
- * Previously workers long-polled for work, holding lots of connections open. Now they short-poll
- * and sleep for a while if there's no work. This is simpler and allows us to work withing much more
- * standard HTTP frameworks.
+ * In our initial design, workers long-polled for work, holding lots of connections open. This was
+ * soon revised to short-poll and sleep for a while when there's no work. This was found to be
+ * simpler, allowing use of standard HTTP frameworks instead of custom networking code.
  * <p>
- * The fact that workers continuously re-poll for work every 10-30 seconds serves as a signal to the
- * broker that they are still alive and waiting. This also allows the broker to maintain a catalog
- * of active workers.
+ * Issue conveyal/r5#596 arose because we'd only poll when a worker was running low on tasks or
+ * idling. A worker could disappear for a long time, leaving the backend to assume it had shut down
+ * or crashed. Workers were revised to poll more frequently even when they were busy and didn't
+ * need any new tasks to work on, providing a signal to the broker that they are still alive and
+ * functioning. This allows the broker to maintain a more accurate catalog of active workers.
  * <p>
- * Because (at least currently) two organizations never share the same graph, we can get by with
- * pulling tasks cyclically or randomly from all the jobs, and actively shape the number of workers
- * with affinity for each graph by forcing some of them to accept tasks on graphs other than the one
- * they have declared affinity for.
+ * Most methods on this class are synchronized because they can be called from many HTTP handler
+ * threads at once (when many workers are polling at once). We should occasionally evaluate whether
+ * synchronizing all methods to make this threadsafe is a performance issue. If so, fine-grained
+ * locking may be advantageous, but as a rule it is much harder to design, test, and maintain.
  * <p>
- * This could be thought of as "affinity homeostasis". We  will constantly keep track of the ideal
+ * Workers were originally intended to migrate from one network to another to handle subsequent jobs
+ * without waiting for more cloud compute instances to start up. In practice we currently assign
+ * each worker a single network, but the balance of workers assigned to each network and the reuse
+ * of workers could in principle be made more sophisticated. The remainder of the comments below
+ * provide context for how this could be refined or improved.
+ *
+ * Because (at least currently) two organizations never share the same graph, we could get by with
+ * pulling tasks cyclically or randomly from all the jobs, and could actively shape the number of
+ * workers with affinity for each graph by forcing some of them to accept tasks on graphs other than
+ * the one they have declared affinity for. If the pool of workers was allowed to grow very large,
+ * we could aim to draw tasks fairly from all organizations, and fairly from all jobs within each
+ * organization.
+ * <p>
+ * We have described this approach as "affinity homeostasis": constantly keep track of the ideal
  * proportion of workers by graph (based on active jobs), and the true proportion of consumers by
  * graph (based on incoming polling), then we can decide when a worker's graph affinity should be
- * ignored and what it should be forced to.
- * <p>
- * It may also be helpful to mark jobs every time they are skipped in the LRU queue. Each time a job
- * is serviced, it is taken out of the queue and put at its end. Jobs that have not been serviced
- * float to the top.
- * <p>
- * Most methods on this class are synchronized, because they can be called from many HTTP handler
- * threads at once.
- *
- * TODO evaluate whether synchronizing all methods to make this threadsafe is a performance issue.
+ * ignored and what graph it should be forced to.
  */
 public class Broker implements Component {
 
@@ -100,7 +105,15 @@ public interface Config {
     private final ListMultimap<WorkerCategory, Job> jobs =
             MultimapBuilder.hashKeys().arrayListValues().build();
 
-    /** The most tasks to deliver to a worker at a time. */
+    /**
+     * The most tasks to deliver to a worker at a time. Workers may request less tasks than this, and the broker should
+     * never send more than the minimum of the two values. 50 tasks gives response bodies of about 65kB. If this value
+     * is too high, all remaining tasks in a job could be distributed to a single worker leaving none for the other
+     * workers, creating a slow-joiner problem especially if the tasks are complicated and slow to complete.
+     *
+     * The value should eventually be tuned. The current value of 16 is just the value used by the previous sporadic
+     * polling system (WorkerStatus.LEGACY_WORKER_MAX_TASKS) which may not be ideal but is known to work.
+     */
     public final int MAX_TASKS_PER_WORKER = 16;
 
     /**
@@ -317,9 +330,13 @@ public void createWorkersInCategory (WorkerCategory category, WorkerTags workerT
 
     /**
      * Attempt to find some tasks that match what a worker is requesting.
-     * Always returns a list, which may be empty if there is nothing to deliver.
+     * Always returns a non-null List, which may be empty if there is nothing to deliver.
+     * Number of tasks in the list is strictly limited to maxTasksRequested.
      */
-    public synchronized List<RegionalTask> getSomeWork (WorkerCategory workerCategory) {
+    public synchronized List<RegionalTask> getSomeWork (WorkerCategory workerCategory, int maxTasksRequested) {
+        if (maxTasksRequested <= 0) {
+            return Collections.EMPTY_LIST;
+        }
         Job job;
         if (config.offline()) {
             // Working in offline mode; get tasks from the first job that has any tasks to deliver.
@@ -335,7 +352,10 @@ public synchronized List<RegionalTask> getSomeWork (WorkerCategory workerCategor
             return Collections.EMPTY_LIST;
         }
         // Return up to N tasks that are waiting to be processed.
-        return job.generateSomeTasksToDeliver(MAX_TASKS_PER_WORKER);
+        if (maxTasksRequested > MAX_TASKS_PER_WORKER) {
+            maxTasksRequested = MAX_TASKS_PER_WORKER;
+        }
+        return job.generateSomeTasksToDeliver(maxTasksRequested);
     }
 
     /**

src/main/java/com/conveyal/analysis/components/broker/Broker.java

@@ -177,7 +177,7 @@ public boolean isErrored () {
     /**
      * @param maxTasks the maximum number of tasks to return.
      * @return some tasks that are not yet marked as completed and have not yet been delivered in
-     *         this delivery pass.
+     *         this delivery pass. May return an empty list, but never null.
      */
     public List<RegionalTask> generateSomeTasksToDeliver (int maxTasks) {
         List<RegionalTask> tasks = new ArrayList<>(maxTasks);