Add logging of max cost exceeded

ibi-group · Nov 21, 2023 · 176f192 · 176f192
1 parent 9e258f5
commit 176f192
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 10 deletions.
diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java
@@ -5,6 +5,7 @@
 import java.util.List;
 import java.util.Optional;
 import org.locationtech.jts.geom.Coordinate;
+import org.opentripplanner.framework.logging.Throttle;
 import org.opentripplanner.framework.tostring.ToStringBuilder;
 import org.opentripplanner.raptor.api.model.RaptorTransfer;
 import org.opentripplanner.routing.algorithm.raptoradapter.transit.cost.RaptorCostConverter;
@@ -13,9 +14,14 @@
 import org.opentripplanner.street.search.request.StreetSearchRequest;
 import org.opentripplanner.street.search.state.EdgeTraverser;
 import org.opentripplanner.street.search.state.StateEditor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class Transfer {
 
+  private static final Logger LOG = LoggerFactory.getLogger(Transfer.class);
+  private static final Throttle THROTTLE_COST_EXCEEDED = Throttle.ofOneSecond();
+
   protected static final int MAX_TRANSFER_COST = 2_000_000;
 
   private final int toStop;
@@ -65,7 +71,9 @@ public Optional<RaptorTransfer> asRaptorTransfer(StreetSearchRequest request) {
     WalkPreferences walkPreferences = request.preferences().walk();
     if (edges == null || edges.isEmpty()) {
       double durationSeconds = distanceMeters / walkPreferences.speed();
-      final double domainCost = costLimitSanityCheck(durationSeconds * walkPreferences.reluctance());
+      final double domainCost = costLimitSanityCheck(
+        durationSeconds * walkPreferences.reluctance()
+      );
       return Optional.of(
         new DefaultRaptorTransfer(
           this.toStop,
@@ -107,8 +115,17 @@ public Optional<RaptorTransfer> asRaptorTransfer(StreetSearchRequest request) {
    * @see EdgeTraverser
    * @see RaptorCostConverter
    */
-  private static int costLimitSanityCheck(double cost) {
-    return (cost >= 0 && cost <= MAX_TRANSFER_COST) ? (int) cost : MAX_TRANSFER_COST;
+  private int costLimitSanityCheck(double cost) {
+    if (cost >= 0 && cost <= MAX_TRANSFER_COST) {
+      return (int) cost;
+    } else {
+      THROTTLE_COST_EXCEEDED.throttle(() ->
+        LOG.warn(
+          "Transfer exceeded maximum cost. Please consider changing the transfer cost calculation. More information: https://github.com/opentripplanner/OpenTripPlanner/pull/5516#issuecomment-1819138078"
+        )
+      );
+      return MAX_TRANSFER_COST;
+    }
   }
 
   @Override

diff --git a/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/TransferTest.java b/src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/TransferTest.java
@@ -21,9 +21,7 @@ class TransferTest {
   private static final IntersectionVertex BRANDENBURG_GATE_V = intersectionVertex(
     Coordinates.BERLIN_BRANDENBURG_GATE
   );
-  private static final IntersectionVertex BOSTON_V = intersectionVertex(
-    Coordinates.BOSTON
-  );
+  private static final IntersectionVertex BOSTON_V = intersectionVertex(Coordinates.BOSTON);
 
   @Nested
   class WithEdges {
@@ -48,11 +46,10 @@ void allowLowCost() {
         StreetSearchRequest.of().build()
       );
       // cost is below max limit and should be included in RAPTOR
-      assertTrue(raptorTransfer.isPresent());
+      assertBelowMaxCost(raptorTransfer.get());
     }
   }
 
-
   @Nested
   class WithoutEdges {
 
@@ -82,12 +79,20 @@ void allowLowCost() {
         StreetSearchRequest.of().build()
       );
       // cost is below max limit and should be included in RAPTOR
-      assertTrue(raptorTransfer.isPresent());
+      assertBelowMaxCost(raptorTransfer.get());
     }
   }
 
   private static void assertMaxCost(RaptorTransfer transfer) {
-    assertEquals(RaptorCostConverter.toRaptorCost(Transfer.MAX_TRANSFER_COST), transfer.generalizedCost());
+    assertEquals(
+      RaptorCostConverter.toRaptorCost(Transfer.MAX_TRANSFER_COST),
+      transfer.generalizedCost()
+    );
   }
 
+  private static void assertBelowMaxCost(RaptorTransfer transfer) {
+    assertTrue(
+      RaptorCostConverter.toRaptorCost(Transfer.MAX_TRANSFER_COST) > transfer.generalizedCost()
+    );
+  }
 }