From eca2528621f37738ab944b2227443b1b3fd0a2c0 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Wed, 23 Oct 2024 11:07:32 +0200 Subject: [PATCH 1/3] Fix max search-window when paging The PagingService was initialized with the dynamic max-search-window value, and not the request max search-window. This commit also updates the Transmodel API doc. --- .../apis/transmodel/TransmodelAPI.java | 11 ++- .../transmodel/TransmodelGraphQLSchema.java | 20 ++++- .../apis/transmodel/model/plan/TripQuery.java | 79 ++++++++++++------- .../mapping/PagingServiceFactory.java | 4 +- .../routing/api/request/RouteRequest.java | 20 ++++- .../standalone/config/RouterConfig.java | 2 +- .../routerconfig/TransitRoutingConfig.java | 2 +- .../configure/ConstructApplication.java | 3 +- .../server/DefaultServerRequestContext.java | 9 ++- .../apis/transmodel/schema.graphql | 37 +++++++-- .../TransmodelGraphQLSchemaTest.java | 7 +- .../routing/core/RouteRequestTest.java | 2 +- 12 files changed, 147 insertions(+), 49 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelAPI.java b/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelAPI.java index 7d982e77885..fe73e8b9887 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelAPI.java +++ b/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelAPI.java @@ -22,6 +22,7 @@ import org.opentripplanner.apis.transmodel.mapping.TransitIdMapper; import org.opentripplanner.routing.api.request.RouteRequest; import org.opentripplanner.standalone.api.OtpServerRequestContext; +import org.opentripplanner.standalone.config.routerconfig.TransitRoutingConfig; import org.opentripplanner.transit.service.TimetableRepository; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -67,14 +68,20 @@ public TransmodelAPIOldPath( public static void setUp( TransmodelAPIParameters config, TimetableRepository timetableRepository, - RouteRequest defaultRouteRequest + RouteRequest defaultRouteRequest, + TransitRoutingConfig transitRoutingConfig ) { if (config.hideFeedId()) { TransitIdMapper.setupFixedFeedId(timetableRepository.getAgencies()); } tracingHeaderTags = config.tracingHeaderTags(); maxNumberOfResultFields = config.maxNumberOfResultFields(); - schema = TransmodelGraphQLSchema.create(defaultRouteRequest, timetableRepository.getTimeZone()); + schema = + TransmodelGraphQLSchema.create( + defaultRouteRequest, + timetableRepository.getTimeZone(), + transitRoutingConfig + ); } @POST diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java b/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java index 5a9fa4cfb59..2ffb9941bc6 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java +++ b/application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java @@ -107,6 +107,7 @@ import org.opentripplanner.model.plan.legreference.LegReference; import org.opentripplanner.model.plan.legreference.LegReferenceSerializer; import org.opentripplanner.routing.alertpatch.TransitAlert; +import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitTuningParameters; import org.opentripplanner.routing.api.request.RouteRequest; import org.opentripplanner.routing.error.RoutingValidationException; import org.opentripplanner.routing.graphfinder.NearbyStop; @@ -133,17 +134,29 @@ public class TransmodelGraphQLSchema { private final DefaultRouteRequestType routing; + private final TransitTuningParameters transitTuningParameters; + private final ZoneId timeZoneId; private final Relay relay = new Relay(); - private TransmodelGraphQLSchema(RouteRequest defaultRequest, ZoneId timeZoneId) { + private TransmodelGraphQLSchema( + RouteRequest defaultRequest, + ZoneId timeZoneId, + TransitTuningParameters transitTuningParameters + ) { this.timeZoneId = timeZoneId; this.routing = new DefaultRouteRequestType(defaultRequest); + this.transitTuningParameters = transitTuningParameters; } - public static GraphQLSchema create(RouteRequest defaultRequest, ZoneId timeZoneId) { - return new TransmodelGraphQLSchema(defaultRequest, timeZoneId).create(); + public static GraphQLSchema create( + RouteRequest defaultRequest, + ZoneId timeZoneId, + TransitTuningParameters transitTuningParameters + ) { + return new TransmodelGraphQLSchema(defaultRequest, timeZoneId, transitTuningParameters) + .create(); } @SuppressWarnings("unchecked") @@ -340,6 +353,7 @@ private GraphQLSchema create() { GraphQLFieldDefinition tripQuery = TripQuery.create( routing, + transitTuningParameters, tripType, durationPerStreetModeInput, penaltyForStreetMode, diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java index 5b1bbd84373..c46b8b36082 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java +++ b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java @@ -12,6 +12,7 @@ import graphql.schema.GraphQLNonNull; import graphql.schema.GraphQLOutputType; import graphql.schema.GraphQLScalarType; +import java.time.Duration; import org.opentripplanner.apis.transmodel.TransmodelGraphQLPlanner; import org.opentripplanner.apis.transmodel.model.DefaultRouteRequestType; import org.opentripplanner.apis.transmodel.model.EnumTypes; @@ -20,6 +21,7 @@ import org.opentripplanner.apis.transmodel.model.framework.PassThroughPointInputType; import org.opentripplanner.apis.transmodel.model.framework.PenaltyForStreetModeType; import org.opentripplanner.apis.transmodel.model.framework.TransmodelDirectives; +import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitTuningParameters; import org.opentripplanner.routing.api.request.preference.RoutingPreferences; import org.opentripplanner.routing.core.VehicleRoutingOptimizeType; @@ -37,6 +39,7 @@ public class TripQuery { public static GraphQLFieldDefinition create( DefaultRouteRequestType routing, + TransitTuningParameters transitTuningParameters, GraphQLOutputType tripType, GraphQLInputObjectType durationPerStreetModeType, GraphQLInputObjectType penaltyForStreetMode, @@ -87,30 +90,42 @@ Normally this is when the search is performed (now), plus a small grace period t .newArgument() .name("searchWindow") .description( - "The length of the search-window in minutes. This parameter is optional." + - "\n\n" + - "The search-window is defined as the duration between the earliest-departure-time(EDT) and " + - "the latest-departure-time(LDT). OTP will search for all itineraries in this departure " + - "window. If `arriveBy=true` the `dateTime` parameter is the latest-arrival-time, so OTP " + - "will dynamically calculate the EDT. Using a short search-window is faster than using a " + - "longer one, but the search duration is not linear. Using a \"too\" short search-window will " + - "waste resources server side, while using a search-window that is too long will be slow." + - "\n\n" + - "OTP will dynamically calculate a reasonable value for the search-window, if not provided. The " + - "calculation comes with a significant overhead (10-20% extra). Whether you should use the " + - "dynamic calculated value or pass in a value depends on your use-case. For a travel planner " + - "in a small geographical area, with a dense network of public transportation, a fixed value " + - "between 40 minutes and 2 hours makes sense. To find the appropriate search-window, adjust it " + - "so that the number of itineraries on average is around the wanted `numItineraries`. Make " + - "sure you set the `numItineraries` to a high number while testing. For a country wide area like " + - "Norway, using the dynamic search-window is the best." + - "\n\n" + - "When paginating, the search-window is calculated using the `numItineraries` in the original " + - "search together with statistics from the search for the last page. This behaviour is " + - "configured server side, and can not be overridden from the client." + - "\n\n" + - "The search-window used is returned to the response metadata as `searchWindowUsed` for " + - "debugging purposes." + """ + The length of the search-window in minutes. This parameter is optional. + + The search-window is defined as the duration between the earliest-departure-time(EDT) and + the latest-departure-time(LDT). OTP will search for all itineraries in this departure + window. If `arriveBy=true` the `dateTime` parameter is the latest-arrival-time, so OTP + will dynamically calculate the EDT. Using a short search-window is faster than using a + longer one, but the search duration is not linear. Using a \"too\" short search-window will + waste resources server side, while using a search-window that is too long will be slow. + + OTP will dynamically calculate a reasonable value for the search-window, if not provided. The + calculation comes with a significant overhead (10-20% extra). Whether you should use the + dynamic calculated value or pass in a value depends on your use-case. For a travel planner + in a small geographical area, with a dense network of public transportation, a fixed value + between 40 minutes and 2 hours makes sense. To find the appropriate search-window, adjust it + so that the number of itineraries on average is around the wanted `numTripPatterns`. Make + sure you set the `numTripPatterns` to a high number while testing. For a country wide area like + Norway, using the dynamic search-window is the best. + + When paginating, the search-window is calculated using the `numTripPatterns` in the original + search together with statistics from the search for the last page. This behaviour is + configured server side, and can not be overridden from the client. The paging may even + exceed the maximum value. + + The search-window used is returned to the response metadata as `searchWindowUsed`. + This can be used by the client to calculate the when the next page start/end. + + Note! In some cases you may have to page many times to get all the results you want. + This is intended. Increasing the search-window beyond the max value is NOT going to be + much faster. Instead the client can inform the user about the progress. + + Maximum value: {max-search-window} + """.replace( + "{max-search-window}", + durationInMinutesToString(transitTuningParameters.maxSearchWindow()) + ) ) .type(Scalars.GraphQLInt) .build() @@ -120,9 +135,12 @@ Normally this is when the search is performed (now), plus a small grace period t .newArgument() .name("pageCursor") .description( - "Use the cursor to go to the next \"page\" of itineraries. Copy the cursor from " + - "the last response and keep the original request as is. This will enable you to " + - "search for itineraries in the next or previous time-window." + """ + Use the cursor to go to the next \"page\" of itineraries. Copy the cursor from the last + response and keep the original request as is. This will enable you to search for + itineraries in the next or previous search-window. The paging will automatically scale + up/down the search-window to fit the `numTripPatterns`. + """ ) .type(Scalars.GraphQLString) .build() @@ -646,4 +664,11 @@ private static String enumValAsString(GraphQLEnumType enumType, Enum otpVal) .get() .getName(); } + + /** + * Format and return: "2440 minute (48h)" + */ + private static String durationInMinutesToString(Duration value) { + return "%d minutes (%s)".formatted(value.toMinutes(), value.toHours() + "h"); + } } diff --git a/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/PagingServiceFactory.java b/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/PagingServiceFactory.java index ec58d444914..97dbda4bb68 100644 --- a/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/PagingServiceFactory.java +++ b/application/src/main/java/org/opentripplanner/routing/algorithm/mapping/PagingServiceFactory.java @@ -24,8 +24,10 @@ public static PagingService createPagingService( ) { return new PagingService( transitTuningParameters.pagingSearchWindowAdjustments(), + // The dynamic search-window is not the same as requested search-window, but in lack + // of a something else we use the raptor dynamic min here. raptorTuningParameters.dynamicSearchWindowCoefficients().minWindow(), - raptorTuningParameters.dynamicSearchWindowCoefficients().maxWindow(), + transitTuningParameters.maxSearchWindow(), searchWindowOf(raptorSearchParamsUsed), edt(searchStartTime, raptorSearchParamsUsed), lat(searchStartTime, raptorSearchParamsUsed), diff --git a/application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java b/application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java index 56ba39d8431..27ecc777050 100644 --- a/application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java +++ b/application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java @@ -11,9 +11,11 @@ import java.util.Collections; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.function.Consumer; import javax.annotation.Nullable; import org.opentripplanner.framework.collection.ListSection; +import org.opentripplanner.framework.lang.ObjectUtils; import org.opentripplanner.framework.time.DateUtils; import org.opentripplanner.framework.tostring.ToStringBuilder; import org.opentripplanner.model.GenericLocation; @@ -26,6 +28,7 @@ import org.opentripplanner.routing.api.response.RoutingError; import org.opentripplanner.routing.api.response.RoutingErrorCode; import org.opentripplanner.routing.error.RoutingValidationException; +import org.opentripplanner.standalone.config.routerconfig.TransitRoutingConfig; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -332,12 +335,25 @@ private boolean hasMaxSearchWindow() { return maxSearchWindow != null; } + /** + * For testing only. Use {@link TransitRoutingConfig#maxSearchWindow()} instead. + * @see #initMaxSearchWindow(Duration) + */ public Duration maxSearchWindow() { return maxSearchWindow; } - public void setMaxSearchWindow(@Nullable Duration maxSearchWindow) { - this.maxSearchWindow = maxSearchWindow; + /** + * Initialize the maxSearchWindow from the transit config. This is necessary because the + * default route request is configured before the {@link TransitRoutingConfig}. + */ + public void initMaxSearchWindow(Duration maxSearchWindow) { + this.maxSearchWindow = + ObjectUtils.requireNotInitialized( + "maxSearchWindow", + this.maxSearchWindow, + Objects.requireNonNull(maxSearchWindow) + ); } public Locale locale() { diff --git a/application/src/main/java/org/opentripplanner/standalone/config/RouterConfig.java b/application/src/main/java/org/opentripplanner/standalone/config/RouterConfig.java index 55128af5659..cb00428284b 100644 --- a/application/src/main/java/org/opentripplanner/standalone/config/RouterConfig.java +++ b/application/src/main/java/org/opentripplanner/standalone/config/RouterConfig.java @@ -69,7 +69,7 @@ public RouterConfig(JsonNode node, String source, boolean logUnusedParams) { this.transmodelApi = new TransmodelAPIConfig("transmodelApi", root); this.routingRequestDefaults = mapDefaultRouteRequest("routingDefaults", root); this.transitConfig = new TransitRoutingConfig("transit", root, routingRequestDefaults); - this.routingRequestDefaults.setMaxSearchWindow(transitConfig.maxSearchWindow()); + this.routingRequestDefaults.initMaxSearchWindow(transitConfig.maxSearchWindow()); this.updatersParameters = new UpdatersConfig(root); this.rideHailingConfig = new RideHailingServicesConfig(root); this.vectorTileConfig = VectorTileConfig.mapVectorTilesParameters(root, "vectorTiles"); diff --git a/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java b/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java index 74dbfd4bab9..8ef190e2f08 100644 --- a/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java +++ b/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java @@ -366,7 +366,7 @@ heuristics perform a Raptor search (one-iteration) to find a trip which we use t .summary("Upper limit for the search-window calculation.") .description( """ -Long search windows consumes a lot of resources and may take a long time. Use this parameter to +Long search windows consumes a lot of resources and may take a long time. Use this parameter to tune the desired maximum search time. This is the parameter that affects the response time most, the downside is that a search is only diff --git a/application/src/main/java/org/opentripplanner/standalone/configure/ConstructApplication.java b/application/src/main/java/org/opentripplanner/standalone/configure/ConstructApplication.java index 560f234187a..979467299cd 100644 --- a/application/src/main/java/org/opentripplanner/standalone/configure/ConstructApplication.java +++ b/application/src/main/java/org/opentripplanner/standalone/configure/ConstructApplication.java @@ -175,7 +175,8 @@ private void setupTransitRoutingServer() { TransmodelAPI.setUp( routerConfig().transmodelApi(), timetableRepository(), - routerConfig().routingRequestDefaults() + routerConfig().routingRequestDefaults(), + routerConfig().transitTuningConfig() ); } diff --git a/application/src/main/java/org/opentripplanner/standalone/server/DefaultServerRequestContext.java b/application/src/main/java/org/opentripplanner/standalone/server/DefaultServerRequestContext.java index 0e81193d787..6dc3d27ce69 100644 --- a/application/src/main/java/org/opentripplanner/standalone/server/DefaultServerRequestContext.java +++ b/application/src/main/java/org/opentripplanner/standalone/server/DefaultServerRequestContext.java @@ -33,7 +33,6 @@ public class DefaultServerRequestContext implements OtpServerRequestContext { private final List rideHailingServices; - private RouteRequest routeRequest = null; private final Graph graph; private final TransitService transitService; private final TransitRoutingConfig transitRoutingConfig; @@ -52,6 +51,8 @@ public class DefaultServerRequestContext implements OtpServerRequestContext { private final StreetLimitationParametersService streetLimitationParametersService; private final LuceneIndex luceneIndex; + private RouteRequest defaultRouteRequestWithTimeSet = null; + /** * Make sure all mutable components are copied/cloned before calling this constructor. */ @@ -142,10 +143,10 @@ public static DefaultServerRequestContext create( @Override public RouteRequest defaultRouteRequest() { // Lazy initialize request-scoped request to avoid doing this when not needed - if (routeRequest == null) { - routeRequest = routeRequestDefaults.copyWithDateTimeNow(); + if (defaultRouteRequestWithTimeSet == null) { + defaultRouteRequestWithTimeSet = routeRequestDefaults.copyWithDateTimeNow(); } - return routeRequest; + return defaultRouteRequestWithTimeSet; } /** diff --git a/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql b/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql index 2f3aef2d9a2..6985ae1d654 100644 --- a/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql +++ b/application/src/main/resources/org/opentripplanner/apis/transmodel/schema.graphql @@ -831,7 +831,12 @@ type QueryType { modes: Modes, "The maximum number of trip patterns to return. Note! This reduce the number of trip patterns AFTER the OTP travel search is done in a post-filtering process. There is little/no performance gain in reducing the number of trip patterns returned. See also the trip meta-data on how to implement paging." numTripPatterns: Int = 50, - "Use the cursor to go to the next \"page\" of itineraries. Copy the cursor from the last response and keep the original request as is. This will enable you to search for itineraries in the next or previous time-window." + """ + Use the cursor to go to the next "page" of itineraries. Copy the cursor from the last + response and keep the original request as is. This will enable you to search for + itineraries in the next or previous search-window. The paging will automatically scale + up/down the search-window to fit the `numTripPatterns`. + """ pageCursor: String, "The list of points the journey is required to pass through." passThroughPoints: [PassThroughPoint!] @deprecated(reason : "Use via instead"), @@ -868,13 +873,35 @@ type QueryType { """ The length of the search-window in minutes. This parameter is optional. - The search-window is defined as the duration between the earliest-departure-time(EDT) and the latest-departure-time(LDT). OTP will search for all itineraries in this departure window. If `arriveBy=true` the `dateTime` parameter is the latest-arrival-time, so OTP will dynamically calculate the EDT. Using a short search-window is faster than using a longer one, but the search duration is not linear. Using a "too" short search-window will waste resources server side, while using a search-window that is too long will be slow. + The search-window is defined as the duration between the earliest-departure-time(EDT) and + the latest-departure-time(LDT). OTP will search for all itineraries in this departure + window. If `arriveBy=true` the `dateTime` parameter is the latest-arrival-time, so OTP + will dynamically calculate the EDT. Using a short search-window is faster than using a + longer one, but the search duration is not linear. Using a "too" short search-window will + waste resources server side, while using a search-window that is too long will be slow. - OTP will dynamically calculate a reasonable value for the search-window, if not provided. The calculation comes with a significant overhead (10-20% extra). Whether you should use the dynamic calculated value or pass in a value depends on your use-case. For a travel planner in a small geographical area, with a dense network of public transportation, a fixed value between 40 minutes and 2 hours makes sense. To find the appropriate search-window, adjust it so that the number of itineraries on average is around the wanted `numItineraries`. Make sure you set the `numItineraries` to a high number while testing. For a country wide area like Norway, using the dynamic search-window is the best. + OTP will dynamically calculate a reasonable value for the search-window, if not provided. The + calculation comes with a significant overhead (10-20% extra). Whether you should use the + dynamic calculated value or pass in a value depends on your use-case. For a travel planner + in a small geographical area, with a dense network of public transportation, a fixed value + between 40 minutes and 2 hours makes sense. To find the appropriate search-window, adjust it + so that the number of itineraries on average is around the wanted `numTripPatterns`. Make + sure you set the `numTripPatterns` to a high number while testing. For a country wide area like + Norway, using the dynamic search-window is the best. - When paginating, the search-window is calculated using the `numItineraries` in the original search together with statistics from the search for the last page. This behaviour is configured server side, and can not be overridden from the client. + When paginating, the search-window is calculated using the `numTripPatterns` in the original + search together with statistics from the search for the last page. This behaviour is + configured server side, and can not be overridden from the client. The paging may even + exceed the maximum value. - The search-window used is returned to the response metadata as `searchWindowUsed` for debugging purposes. + The search-window used is returned to the response metadata as `searchWindowUsed`. + This can be used by the client to calculate the when the next page start/end. + + Note! In some cases you may have to page many times to get all the results you want. + This is intended. Increasing the search-window beyond the max value is NOT going to be + much faster. Instead the client can inform the user about the progress. + + Maximum value: 1440 minutes (24h) """ searchWindow: Int, """ diff --git a/application/src/test/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchemaTest.java b/application/src/test/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchemaTest.java index bfbaedfabbf..4cdb0586aa7 100644 --- a/application/src/test/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchemaTest.java +++ b/application/src/test/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchemaTest.java @@ -9,6 +9,7 @@ import java.io.File; import org.junit.jupiter.api.Test; import org.opentripplanner._support.time.ZoneIds; +import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitTuningParameters; import org.opentripplanner.routing.api.request.RouteRequest; class TransmodelGraphQLSchemaTest { @@ -19,7 +20,11 @@ class TransmodelGraphQLSchemaTest { @Test void testSchemaBuild() { - var schema = TransmodelGraphQLSchema.create(new RouteRequest(), ZoneIds.OSLO); + var schema = TransmodelGraphQLSchema.create( + new RouteRequest(), + ZoneIds.OSLO, + TransitTuningParameters.FOR_TEST + ); assertNotNull(schema); String original = readFile(SCHEMA_FILE); diff --git a/application/src/test/java/org/opentripplanner/routing/core/RouteRequestTest.java b/application/src/test/java/org/opentripplanner/routing/core/RouteRequestTest.java index b18c12e0485..15960c44122 100644 --- a/application/src/test/java/org/opentripplanner/routing/core/RouteRequestTest.java +++ b/application/src/test/java/org/opentripplanner/routing/core/RouteRequestTest.java @@ -133,7 +133,7 @@ void testZeroSearchWindow() { @Test void testTooLongSearchWindow() { RouteRequest request = new RouteRequest(); - request.setMaxSearchWindow(DURATION_24_HOURS); + request.initMaxSearchWindow(DURATION_24_HOURS); assertThrows( IllegalArgumentException.class, () -> request.setSearchWindow(DURATION_24_HOURS_AND_ONE_MINUTE) From 685ea174064064abd3112c519fb35c87f6742774 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Wed, 23 Oct 2024 15:30:12 +0200 Subject: [PATCH 2/3] review: Apply review fixes. --- .../apis/transmodel/model/plan/TripQuery.java | 10 ++++---- .../DynamicSearchWindowCoefficients.java | 5 ++-- .../routerconfig/TransitRoutingConfig.java | 24 +++++++++---------- doc/user/RouterConfiguration.md | 2 +- 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java index c46b8b36082..09ae8a593b5 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java +++ b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java @@ -101,7 +101,7 @@ Normally this is when the search is performed (now), plus a small grace period t waste resources server side, while using a search-window that is too long will be slow. OTP will dynamically calculate a reasonable value for the search-window, if not provided. The - calculation comes with a significant overhead (10-20% extra). Whether you should use the + calculation comes with a significant overhead (10-20%% extra). Whether you should use the dynamic calculated value or pass in a value depends on your use-case. For a travel planner in a small geographical area, with a dense network of public transportation, a fixed value between 40 minutes and 2 hours makes sense. To find the appropriate search-window, adjust it @@ -121,10 +121,10 @@ calculation comes with a significant overhead (10-20% extra). Whether you should This is intended. Increasing the search-window beyond the max value is NOT going to be much faster. Instead the client can inform the user about the progress. - Maximum value: {max-search-window} - """.replace( - "{max-search-window}", - durationInMinutesToString(transitTuningParameters.maxSearchWindow()) + Maximum value: %d minutes (%dh) + """.formatted( + transitTuningParameters.maxSearchWindow().toMinutes(), + transitTuningParameters.maxSearchWindow().toHours() ) ) .type(Scalars.GraphQLInt) diff --git a/application/src/main/java/org/opentripplanner/raptor/api/request/DynamicSearchWindowCoefficients.java b/application/src/main/java/org/opentripplanner/raptor/api/request/DynamicSearchWindowCoefficients.java index 27ebb62c870..4cc74c34c66 100644 --- a/application/src/main/java/org/opentripplanner/raptor/api/request/DynamicSearchWindowCoefficients.java +++ b/application/src/main/java/org/opentripplanner/raptor/api/request/DynamicSearchWindowCoefficients.java @@ -60,11 +60,10 @@ default Duration minWindow() { /** * Set an upper limit to the calculation of the dynamic search window to prevent exceptionable - * cases to cause very long search windows. Long search windows consumes a lot of resources and + * cases to cause very long search windows. Long search windows consume a lot of resources and * may take a long time. Use this parameter to tune the desired maximum search time. *

- * This is the parameter that affect the response time most, the downside is that a search is only - * guaranteed to be pareto-optimal within a search-window. + * This is the parameter that affects the response time the most. *

* The default is 3 hours. The unit is minutes. */ diff --git a/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java b/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java index 8ef190e2f08..de67cf03817 100644 --- a/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java +++ b/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java @@ -62,8 +62,8 @@ public TransitRoutingConfig( .summary("This parameter is used to allocate enough memory space for Raptor.") .description( """ -Set it to the maximum number of transfers for any given itinerary expected to be found within the -entire transit network. The memory overhead of setting this higher than the maximum number of +Set it to the maximum number of transfers for any given itinerary expected to be found within the +entire transit network. The memory overhead of setting this higher than the maximum number of transfers is very little so it is better to set it too high than to low. """ ) @@ -78,8 +78,8 @@ public TransitRoutingConfig( .description( """ This reduce the number of trips departure time lookups and comparisons. When testing with data from -Entur and all of Norway as a Graph, the optimal value was about 50. If you calculate the departure -time every time or want to fine tune the performance, changing this may improve the performance a +Entur and all of Norway as a Graph, the optimal value was about 50. If you calculate the departure +time every time or want to fine tune the performance, changing this may improve the performance a few percents. """ ) @@ -108,7 +108,7 @@ public TransitRoutingConfig( .description( """ Use this parameter to set the total number of executable threads available across all searches. -Multiple searches can run in parallel - this parameter have no effect with regard to that. If 0, +Multiple searches can run in parallel - this parameter have no effect with regard to that. If 0, no extra threads are started and the search is done in one thread. """ ) @@ -142,7 +142,7 @@ public TransitRoutingConfig( | `recommended` | Use a small cost penalty like `60`. | int | | `preferred` | The best place to do transfers. Should be set to `0`(zero). | int | -Use values in a range from `0` to `100 000`. **All key/value pairs are required if the +Use values in a range from `0` to `100 000`. **All key/value pairs are required if the `stopBoardAlightDuringTransferCost` is listed.** """ ) @@ -166,7 +166,7 @@ public TransitRoutingConfig( .summary("Routing requests to use for pre-filling the stop-to-stop transfer cache.") .description( """ -If not set, the default behavior is to cache stop-to-stop transfers using the default route request +If not set, the default behavior is to cache stop-to-stop transfers using the default route request (`routingDefaults`). Use this to change the default or specify more than one `RouteRequest`. **Example** @@ -175,7 +175,7 @@ public TransitRoutingConfig( // router-config.json { "transit": { - "transferCacheRequests": [ + "transferCacheRequests": [ { "modes": "WALK" }, { "modes": "WALK", "wheelchairAccessibility": { "enabled": true } } ] @@ -201,7 +201,7 @@ public TransitRoutingConfig( """ The search window is expanded when the current page return few options. If ZERO result is returned the first duration in the list is used, if ONE result is returned then the second duration is used -and so on. The duration is added to the existing search-window and inserted into the next and +and so on. The duration is added to the existing search-window and inserted into the next and previous page cursor. See JavaDoc for [TransitTuningParameters#pagingSearchWindowAdjustments](https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/main/java/org/opentripplanner/routing/algorithm/raptor/transit/TransitTuningParameters.java)" + for more info." """ @@ -366,7 +366,7 @@ heuristics perform a Raptor search (one-iteration) to find a trip which we use t .summary("Upper limit for the search-window calculation.") .description( """ -Long search windows consumes a lot of resources and may take a long time. Use this parameter to +Long search windows consume a lot of resources and may take a long time. Use this parameter to tune the desired maximum search time. This is the parameter that affects the response time most, the downside is that a search is only @@ -381,12 +381,12 @@ heuristics perform a Raptor search (one-iteration) to find a trip which we use t .summary("Used to set the steps the search-window is rounded to.") .description( """ -The search window is rounded off to the closest multiplication of `stepMinutes`. If `stepMinutes` = +The search window is rounded off to the closest multiplication of `stepMinutes`. If `stepMinutes` = 10 minutes, the search-window can be 10, 20, 30 ... minutes. It the computed search-window is 5 minutes and 17 seconds it will be rounded up to 10 minutes. -Use a value between `1` and `60`. This should be less than the `min-raptor-search-window` +Use a value between `1` and `60`. This should be less than the `min-raptor-search-window` coefficient. """ ) diff --git a/doc/user/RouterConfiguration.md b/doc/user/RouterConfiguration.md index 766ad0de4ef..425ab92373e 100644 --- a/doc/user/RouterConfiguration.md +++ b/doc/user/RouterConfiguration.md @@ -295,7 +295,7 @@ In addition there is an upper bound on the calculation of the search window: Upper limit for the search-window calculation. -Long search windows consumes a lot of resources and may take a long time. Use this parameter to +Long search windows consume a lot of resources and may take a long time. Use this parameter to tune the desired maximum search time. This is the parameter that affects the response time most, the downside is that a search is only From 4d3fff12ce3bafe80f2b865a5c9db0071c52e317 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Thu, 24 Oct 2024 10:56:25 +0200 Subject: [PATCH 3/3] review: Apply review fixes. --- .../apis/transmodel/model/plan/TripQuery.java | 8 -------- .../config/routerconfig/TransitRoutingConfig.java | 2 +- doc/user/RouterConfiguration.md | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java index 09ae8a593b5..ac496131954 100644 --- a/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java +++ b/application/src/main/java/org/opentripplanner/apis/transmodel/model/plan/TripQuery.java @@ -12,7 +12,6 @@ import graphql.schema.GraphQLNonNull; import graphql.schema.GraphQLOutputType; import graphql.schema.GraphQLScalarType; -import java.time.Duration; import org.opentripplanner.apis.transmodel.TransmodelGraphQLPlanner; import org.opentripplanner.apis.transmodel.model.DefaultRouteRequestType; import org.opentripplanner.apis.transmodel.model.EnumTypes; @@ -664,11 +663,4 @@ private static String enumValAsString(GraphQLEnumType enumType, Enum otpVal) .get() .getName(); } - - /** - * Format and return: "2440 minute (48h)" - */ - private static String durationInMinutesToString(Duration value) { - return "%d minutes (%s)".formatted(value.toMinutes(), value.toHours() + "h"); - } } diff --git a/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java b/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java index de67cf03817..e6602a188f4 100644 --- a/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java +++ b/application/src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java @@ -108,7 +108,7 @@ public TransitRoutingConfig( .description( """ Use this parameter to set the total number of executable threads available across all searches. -Multiple searches can run in parallel - this parameter have no effect with regard to that. If 0, +Multiple searches can run in parallel - this parameter has no effect with regard to that. If 0, no extra threads are started and the search is done in one thread. """ ) diff --git a/doc/user/RouterConfiguration.md b/doc/user/RouterConfiguration.md index 425ab92373e..6dbd1174397 100644 --- a/doc/user/RouterConfiguration.md +++ b/doc/user/RouterConfiguration.md @@ -237,7 +237,7 @@ few percents. Split a travel search in smaller jobs and run them in parallel to improve performance. Use this parameter to set the total number of executable threads available across all searches. -Multiple searches can run in parallel - this parameter have no effect with regard to that. If 0, +Multiple searches can run in parallel - this parameter has no effect with regard to that. If 0, no extra threads are started and the search is done in one thread.