Merge branch 'master' into test/large-feeds

MobilityData · Oct 9, 2024 · f9ba38e · f9ba38e
2 parents 8fa5651 + fa91ddc
commit f9ba38e
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 37 deletions.
diff --git a/docs/ACCEPTANCE_TESTS.md b/docs/ACCEPTANCE_TESTS.md
@@ -106,7 +106,7 @@ We follow this process:
 
 ## Performance metrics within the acceptance reports
 
-There are two man metrics added to the acceptance report comment at the PR level, _Validation Time_ and _Memory Consumption_.
+There are two main metrics added to the acceptance report comment at the PR level, _Validation Time_ and _Memory Consumption_.
 The performance metrics are **not a blocker** as performance might vary due to external factors including GitHub infrastructure performance.
 However, large jumps in performance values should be investigated before approving a PR.
 
@@ -115,6 +115,11 @@ The validation time consists in general metrics like average, median, standard d
 This metrics can be affected by addition of new validators than introduce a penalty in processing time.
 
 ### Memory Consumption
+There are two main patterns on how to take a memory usage snapshot:
+
+- MemoryMonitor annotation: This annotation persists the memory usage in the target method. As a limitation, for methods that have concurrent thread executions, the annotation persists in multiple snapshots. This cannot be very clear when analyzing memory usage.
+- MemoryUsageRegister: using the registry directly give you more flexibility than the annotation and can be used in cases where MemoryMonitor produces multiple entries on concurrent executed methods.
+
 The memory consumption section contains three tables.
 - The first, list the first 25 datasets that the difference increased memory comparing with the main branch.
 - The second, list the first 25 datasets that the difference decreased memory comparing with the main branch.

diff --git a/docs/FEATURES.md b/docs/FEATURES.md
diff --git a/main/src/main/java/org/mobilitydata/gtfsvalidator/runner/ValidationRunner.java b/main/src/main/java/org/mobilitydata/gtfsvalidator/runner/ValidationRunner.java
@@ -76,6 +76,8 @@ public ValidationRunner(VersionResolver versionResolver) {
   @MemoryMonitor
   public Status run(ValidationRunnerConfig config) {
     MemoryUsageRegister.getInstance().clearRegistry();
+    // Registering the memory metrics manually to avoid multiple entries due to concurrent calls
+    // and exclude from the metric the generation of the reports.
     var memoryBefore =
         MemoryUsageRegister.getInstance().getMemoryUsageSnapshot("ValidationRunner.run", null);
     VersionInfo versionInfo =

diff --git a/main/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsStopTimeSchema.java b/main/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsStopTimeSchema.java
@@ -29,7 +29,6 @@
 import org.mobilitydata.gtfsvalidator.annotation.Index;
 import org.mobilitydata.gtfsvalidator.annotation.NonNegative;
 import org.mobilitydata.gtfsvalidator.annotation.PrimaryKey;
-import org.mobilitydata.gtfsvalidator.annotation.RecommendedColumn;
 import org.mobilitydata.gtfsvalidator.annotation.Required;
 import org.mobilitydata.gtfsvalidator.type.GtfsTime;
 
@@ -91,7 +90,6 @@ public interface GtfsStopTimeSchema extends GtfsEntity {
   double shapeDistTraveled();
 
   @DefaultValue("1")
-  @RecommendedColumn
   GtfsStopTimeTimepoint timepoint();
 
   @FieldType(FieldTypeEnum.ID)

diff --git a/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java b/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java
@@ -60,7 +60,7 @@ public void validate(NoticeContainer noticeContainer) {
       return;
     }
     for (GtfsStopTime stopTime : stopTimes.getEntities()) {
-      if (!stopTime.hasTimepoint()) {
+      if ((stopTime.hasArrivalTime() || stopTime.hasDepartureTime()) && !stopTime.hasTimepoint()) {
         noticeContainer.addValidationNotice(new MissingTimepointValueNotice(stopTime));
       }
       if (isTimepoint(stopTime)) {

diff --git a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java
@@ -201,7 +201,8 @@ public void nonTimepoint_timesProvided_shouldNotGenerateNotice() {
   }
 
   @Test
-  public void emptyTimepoint_noTimesProvided_shouldGenerateNotice() {
+  public void
+      emptyTimepoint_noArrivalTime_noDepartureTime_noTimePointProvided_shouldNotGenerateNotice() {
     // setting .setTimepoint(null) is used to define a missing value
     // (even if the timepoint value is included in header)
     List<GtfsStopTime> stopTimes = new ArrayList<>();
@@ -216,7 +217,7 @@ public void emptyTimepoint_noTimesProvided_shouldGenerateNotice() {
             .setTimepoint((Integer) null)
             .build());
     assertThat(generateNotices(createHeaderWithTimepointColumn(), stopTimes))
-        .containsExactly(new MissingTimepointValueNotice(stopTimes.get(0)));
+        .doesNotContain(new MissingTimepointValueNotice(stopTimes.get(0)));
   }
 
   @Test

diff --git a/.../src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/io/DatasetMemoryUsage.java b/.../src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/io/DatasetMemoryUsage.java
@@ -1,5 +1,7 @@
 package org.mobilitydata.gtfsvalidator.outputcomparator.io;
 
+import com.google.common.flogger.FluentLogger;
+
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -13,6 +15,8 @@
  */
 public class DatasetMemoryUsage {
 
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   private String datasetId;
   private List<MemoryUsage> referenceMemoryUsage;
   private List<MemoryUsage> latestMemoryUsage;
@@ -29,12 +33,28 @@ public DatasetMemoryUsage(
     if (referenceMemoryUsage != null) {
       this.referenceUsedMemoryByKey =
           referenceMemoryUsage.stream()
-              .collect(Collectors.toUnmodifiableMap(MemoryUsage::getKey, MemoryUsage::usedMemory));
+              .collect(
+                  Collectors.toUnmodifiableMap(
+                      MemoryUsage::getKey,
+                      MemoryUsage::usedMemory,
+                      (existing, replacement) -> {
+                        logger.atWarning().log(
+                            "Duplicate key found in referenceMemoryUsage: " + existing);
+                        return existing;
+                      }));
     }
     if (latestMemoryUsage != null) {
       this.latestUsedMemoryByKey =
           latestMemoryUsage.stream()
-              .collect(Collectors.toUnmodifiableMap(MemoryUsage::getKey, MemoryUsage::usedMemory));
+              .collect(
+                  Collectors.toUnmodifiableMap(
+                      MemoryUsage::getKey,
+                      MemoryUsage::usedMemory,
+                      (existing, replacement) -> {
+                        logger.atWarning().log(
+                            "Duplicate key found in latestMemoryUsage: " + existing);
+                        return existing;
+                      }));
     }
   }