Upstream merge 2022-09-23

.github/workflows/close_stale_pr_and_issues.yml

@@ -19,6 +19,6 @@ jobs:
           stale-issue-message: 'This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days'
           days-before-stale: 90
           days-before-close: 30
-          operations-per-run: 240
+          operations-per-run: 260
           exempt-issue-labels: 'Roadmap'
           ascending: true

.github/workflows/performance-test.yml

@@ -7,22 +7,31 @@ on:
 
 jobs:
   perf-test:
-    if: github.repository_owner == 'opentripplanner'
+    if: github.repository_owner == 'opentripplanner' && !startsWith(github.event.head_commit.message ,'Bump serialization version id for')
     runs-on: performance-test
     strategy:
-      max-parallel: 1
       matrix:
         include:
 
-          - location: baden-wuerttemberg # German state of Baden-Württemberg: https://en.wikipedia.org/wiki/Baden-W%C3%BCrttemberg
-            osm-url: https://download.geofabrik.de/europe/germany/baden-wuerttemberg-220101.osm.pbf
-            transit-url: https://leonard.io/otp/baden-wuerttemberg-2022-07-25.gtfs.tidy.zip
-            transit-filename: baden-wuerttemberg.gtfs.zip
+          - location: germany # all of Germany (500k stops, 200k patterns) but no OSM
+            transit-url: https://leonard.io/otp/germany-2022-08-23.tidy.gtfs.zip
+            transit-filename: germany.gtfs.zip
+            iterations: 1
+            jfr-delay: "50s"
 
           - location: norway
             osm-url: https://download.geofabrik.de/europe/norway-210101.osm.pbf
             transit-url: https://leonard.io/otp/rb_norway-aggregated-netex-2021-12-11.zip
             transit-filename: rb_norway-aggregated-netex.zip
+            iterations: 4
+            jfr-delay: "35s"
+
+          - location: baden-wuerttemberg # German state of Baden-Württemberg: https://en.wikipedia.org/wiki/Baden-W%C3%BCrttemberg
+            osm-url: https://download.geofabrik.de/europe/germany/baden-wuerttemberg-220101.osm.pbf
+            transit-url: https://leonard.io/otp/baden-wuerttemberg-2022-07-25.gtfs.tidy.zip
+            transit-filename: baden-wuerttemberg.gtfs.zip
+            iterations: 4
+            jfr-delay: "50s"
 
     steps:
       - uses: actions/[email protected]
@@ -44,24 +53,27 @@ jobs:
         with:
           path: ~/.m2/repository
           key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
-          restore-keys: |
-            ${{ runner.os }}-maven
 
       - name: Cache graph input files
         uses: actions/cache@v2
         with:
           path: |
             graph/*.pbf
             graph/*.zip
-          key: graph-${{ matrix.location }}
+          key: graph-${{ matrix.transit-url}}-${{ matrix.osm-url}}
 
       - name: Build jar
         run: mvn -DskipTests --batch-mode package -P prettierSkip
 
       - name: Download OSM & Netex data
         run: |
           mkdir -p graph
-          wget ${{ matrix.osm-url }} -O graph/osm.pbf --no-clobber -q --show-progress --progress=bar:force || true
+          if [ "${{ matrix.osm-url }}" = "" ];
+          then
+            echo "No OSM download specified. Skipping..."
+          else
+            wget ${{ matrix.osm-url }} -O graph/osm.pbf --no-clobber -q --show-progress --progress=bar:force || true
+          fi
           wget ${{ matrix.transit-url }} -O graph/${{ matrix.transit-filename }} --no-clobber -q --show-progress --progress=bar:force || true
 
       - name: Build graph
@@ -75,14 +87,18 @@ jobs:
           PERFORMANCE_INFLUX_DB_PASSWORD: ${{ secrets.PERFORMANCE_INFLUX_DB_PASSWORD }}
           MEASUREMENT_ENVIRONMENT: CI
           SPEEDTEST_LOCATION: ${{ matrix.location }}
+          MAVEN_OPTS: "-Xmx50g -XX:StartFlightRecording=delay=${{ matrix.jfr-delay }},duration=30m,filename=${{ matrix.location}}-speed-test.jfr"
         run: |
           mv graph/graph.obj test/performance/${{ matrix.location }}
-          mvn exec:java -Dexec.mainClass="org.opentripplanner.transit.raptor.speed_test.SpeedTest" -Dexec.classpathScope=test -Dexec.args="--dir=test/performance/${{ matrix.location }} -p md -n 4 -i 3 -0"
-
+          mvn exec:java -Dexec.mainClass="org.opentripplanner.transit.raptor.speed_test.SpeedTest" -Dexec.classpathScope=test -Dexec.args="--dir=test/performance/${{ matrix.location }} -p md -n ${{ matrix.iterations }} -i 3 -0" -P prettierSkip
       - name: Archive travel results file
-        uses: actions/upload-artifact@v2
+        uses: actions/upload-artifact@v3
         with:
           name: ${{ matrix.location }}-travelSearch-results.csv
-          path: |
-            test/performance/${{ matrix.location }}/travelSearch-results.csv
+          path: test/performance/${{ matrix.location }}/travelSearch-results.csv
 
+      - name: Archive Flight Recorder instrumentation file
+        uses: actions/upload-artifact@v3
+        with:
+          name: ${{ matrix.location }}-flight-recorder
+          path: ${{ matrix.location}}-speed-test.jfr

CODE_CONVENTIONS.md

@@ -0,0 +1,205 @@
+# Code Conventions
+
+We try to follow these conventions or best practices. The goal is to get cleaner code and make the
+review process easier:
+
+- the developer knows what to expect
+- the reviewer knows what to look for
+- discussions and personal preferences can be avoided saving time
+- new topics should be documented here
+
+These conventions are not "hard" rules, and often there might be other forces which pull a
+decision in another direction, in that case documenting your choice is often enough to pass the
+review.
+
+## Best practices - in focus
+
+- [ ] Document `public` interfaces, classes and methods - especially those part of a module api.
+- [ ] Leave Things BETTER than you found them - clean up code you visit or/and add unit tests.
+- [ ] [Feature envy](https://refactoring.guru/smells/feature-envy)
+- [ ] Make types immutable if possible. References to other Entities might need to be mutable, if
+      so try to init them once, and throw an exception if set again.
+      Example:
+
+```java
+Builder initStop(Stop stop) {
+   this.stop = requireNotInitialized(this.stop, stop);
+}
+```
+
+## Naming Conventions
+
+### Packages
+
+Try to arrange code by domain functionality, not technology. The main structure of a package should
+be `org.opentripplanner.<domain>.<component>.<sub-component>`.
+
+| Package                         | Description                                                                                                                                                                                              |
+| ------------------------------- |----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `o.o.<domain>`                  | At the top level we should divide OTP into "domain"s like `apis`, `framework`, `transit`, `street`, `astar`, `raptor`, `feeds`, `updaters`, and `application`.                                           |
+| `component` and `sub-component` | A group of packages/classes witch naturally belong together, think aggregate as in Domain Driven Design.                                                                                                 |
+| `component.api`                 | Used for components to define the programing interface for the component. If present, (see Raptor) all outside dependencies to the component should be through the `api`.                                |
+| `component.model`               | Used to create a model of a Entites, ValueObjects, ++. If exposed outside the component you should include an entry point like `xyz.model.XyzModel` and/or a Service (in api or component root package). |
+| `component.service`             | Implementation of the service like `DefaultTransitService`, may also contain use-case specific code. Note, the Service interface goes into the component root or `api`, not in the service package.      |
+| `component.configure`           | Component creation/orchestration. Put Dependency Injection code here, like the Dagger Module.                                                                                                            |
+| `support`                       | Sometimes domain logic get complicated, then extracting/isolating it helps. `support` is used internally in a component, not outside.                                                                    |
+| `framework`                     | (Abstract) building blocks internal to a domain/parent package. In some cases accessed outside the component, e.g. `OptAppException`, `TransitEntity`.                                                   |
+| `mapping`                       | Map between two domains/components.                                                                                                                                                                      |
+| `util`                          | General "util" functionality, often characterized by `static` methods. Dependencies to other OTP packages is NOT allowed, only 3rd party utils libs.                                                     |
+| `o.o.apis`                      | OTP external endpoints. Note! Many apis are in the Sandbox where they are in the `o.o.ext` package.                                                                                                      |
+
+> **Note!** The above is the goal, the current package structure needs cleanup. 
+
+> **Note!** Util methods depending on an OTP type/component should go into that type/component, not in the
+utils class. E.g. static factory methods. Warning the "pure" utilities right now are placed into 
+sub-packages of `o.o.util`, the root package needs cleanup.
+
+
+### Methods
+
+Here are a list of common prefixes used, and what to expect.
+
+| Good method prefixes                                  | Description                                                                 |
+|-------------------------------------------------------|-----------------------------------------------------------------------------|
+| `stop() : Stop`                                       | Field accessor, equivalent to `getStop` as in the Java Bean standard        |
+| `getStop(ID id) : Stop`                               | Get Stop by ID, throws exception if not found                               |
+| `getStops(Collection<ID> id) : List/Collection<Stop>` | Get ALL Stops by set of IDs, throws exception if not found                  |
+| `findStop(Criteria criteria) : Optional<Stop>`        | Find one or zero stops, return `Optional`                                   |
+| `findStops(Criteria criteria) : List/Stream<Stop>`    | Find 0, 1 or many stops, return a collection or stream(List is preferred)   |
+| `listStops() : List/Stream<Stop>`                     | List ALL stops in context, return a collection or stream(List is preferred) |
+| `withStop(Stop stop) : Builder`                       | Set stop in builder, replacing existing value and return `this` builder     |
+| `initStop(Stop stop) : void`                          | Set property ONCE, a second call throws an exception                        |
+| `addStop(Stop stop) : void/Builder`                   | Add a stop to a collection of stops.                                        |
+| `addStops(Collection<Stop> stops) : void/Builder`     | Add set of stops to existing set.                                           |
+| `withBike(Consumer<BikePref.Builder> body) : Builder` | For nested builders use lambdas.                                            |
+
+These prefixes are also "allowed", but not preferred - they have some kind of negative "force" to them.
+
+| Ok method prefixes, but ...                 | Description                                                                                           |
+| ------------------------------------------- | ----------------------------------------------------------------------------------------------------- |
+| `withStops(Collection<Stop> stops) : this`) | Replace all stops in builder with new set, consider using `addStops(...)` instead                     |
+| `setStop(Stop stop)`                        | Set a mutable stop reference. Avoid if not part of natural lifecycle. Use `initStop(...)` if possible |
+| `getStop() : Stop`                          | Old style accessor, use the shorter form `stop() : Stop`                                              |
+
+### Services, Models, Editors and Builders
+
+Naming convention for builders with and without a context.
+
+##### Graph Build and tests run without a context
+
+```Java
+// Create a new Stop
+trip = Trip.of(id).withName("The Express").build();
+
+// Modify and existing stop
+stop = stop.copyOf().withPrivateCode("TEX").build();
+```
+
+
+## Records, POJOs and Builders
+
+We prefer immutable typesafe types over flexibility and "short" class definitions. This make
+the code more robust and less error-prune.
+
+### Records
+
+You may use records, but avoid using records if you can not encapsulate it properly. Be especially
+aware of arrays fields (can not be protected) and collections (remember to make a defensive copy).
+If you need to override `equals` and `hashCode`, then it is probably not worth it.
+Be aware that `equals` compare references, not the value of a field. Consider overriding `toString`.
+
+### Builders
+
+OTP used a simple builder pattern in many places, especially when creating immutable types.
+
+#### Builder conventions
+- Use factory methods to create builder, either `of()` or `copyOf()`. The _copyOf_ uses an existing
+  instance as its base. The `of()` creates a builder with all default values set. All constructors 
+  should be private (or package local) to enforce the use of the factory methods.
+- If the class have more than 5 fields avoid using an inner class builder, instead create a builder
+  in the same package.
+- Make all fields in the main class final to enforce immutability.
+- Consider using utility methods for parameter checking, like `Objects#requireNonNull` and 
+  `ObjectUtils.ifNotNull`.
+- Validate all fields in the main type constructor(i.e. not in the builder), especially null checks. 
+  Prefer default values over null-checks. All business logic using the type can rely on its validity.
+- You may keep the original instance in the builder to avoid creating a new object if nothing 
+  changed. This prevents polluting the heap for long-lived objects and make comparison very fast.
+- There is no need to provide all get accessors in the Builder if not needed.
+- Unit-test builders and verify all fields are copied over.
+- For nested builders see the field `nested` in the example.
+
+<details>
+    <summary><b>Builder example</b></summary>
+
+```Java
+/**
+ * THIS CLASS IS IMMUTABLE AND THREAD-SAFE
+ */
+public class A {
+  public static final A DEFAULT = new A();
+  private final List<String> names;
+  private final int age;
+  private final B nested;
+
+  private A() {
+    this.names = List.of("default");
+    this.age = 7;
+    this.nested = B.of();
+  }
+
+  private A(Builder builder) {
+    this.names = List.copyOf(builder.names);
+    this.age = builder.age;
+    this.nested = builder.nested();
+
+    if(age < 0 || age > 150) {
+      throw new IllegalArgumentException("Age is out of range[0..150]: " + age);
+    }
+  }
+
+  public static A.Builder of() { return DEFAULT.copyOf(); }
+  public A.Builder copyOf() { return new Builder(this); }
+
+  public List<String> listNames() { return names; }
+  public int age()                { return age; }
+
+  public boolean equals(Object other) { ... }
+  public int hashCode()               { ... }
+  public String toString()            { return ToStringBuilder.of(A.class)...; }
+
+  public static class Builder {
+    private final A original;
+    private final List<String> names;
+    private int age;
+    private B.Builder nested = null;
+
+    public Builder(A original) {
+      this.original = original;
+      this.names = new ArrayList<>(original.names);
+      this.age = original.age;
+    }
+
+    public Builder withName(String name) { this.names.add(name); return this; }
+
+    public int age()                     { return age; }
+    public Builder withAge(int age)      { this.age = age; return this; }
+
+    private B nested() { return nested==null ? original.nested() : nested.build(); } 
+    public Builder withB(Consumer<B.Builder> body) {
+      if(nested == null) { nested = original.nested.copyOf(); } 
+      body.accept(nested);
+      return this;
+    }
+    public A build() {
+      A value = new A(this);
+      return original.equals(value) ? original : value;
+    }
+  }
+}
+```
+
+</details>
+
+
+