Skip to content

Commit 83a3929

Browse files
committed
improvement(Addressed PR feedback):
1 parent 370b991 commit 83a3929

File tree

8 files changed

+16
-27
lines changed

8 files changed

+16
-27
lines changed

Diff for: README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -309,8 +309,8 @@ The special E2E client settings should be defined in `env.yml`:
309309
| OTP_ADMIN_DASHBOARD_NAME | string | Optional | OTP Admin Dashboard | Config setting for linking to the OTP Admin Dashboard. |
310310
| OTP_ADMIN_DASHBOARD_URL | string | Optional | https://admin.example.com | Config setting for linking to the OTP Admin Dashboard. |
311311
| OTP_API_ROOT | string | Required | http://otp-server.example.com/otp | The URL of an operational OTP (v2.x) server. |
312-
| OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS | integer | Optional | 60 | The maximum time for making requests to OTP as part of itinerary existence checks. |
313312
| OTP_REQUESTS_THREADING_ENABLED | string | Optional | true | Use multi-threading to handle OTP requests and responses. |
313+
| OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS | integer | Optional | 30 | The maximum time for making requests to OTP. |
314314
| OTP_TIMEZONE | string | Required | America/Los_Angeles | The timezone identifier that OTP is using to parse dates and times. OTP will use the timezone identifier that it finds in the first available agency to parse dates and times. |
315315
| OTP_UI_NAME | string | Optional | Trip Planner | Config setting for linking to the OTP UI (trip planner). |
316316
| OTP_UI_URL | string | Optional | https://plan.example.com | Config setting for linking to the OTP UI (trip planner). |

Diff for: configurations/default/env.yml.tmp

+2-2
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,8 @@ TRUSTED_COMPANION_CONFIRMATION_PAGE_URL: https://otp-server.example.com/trusted/
119119
# A window used in conjunction with tracking update frequency to define a consecutive deviation threshold.
120120
CONSECUTIVE_DEVIATIONS_WINDOW_SECONDS: 30
121121

122-
# The maximum time for making requests to OTP as part of itinerary existence checks.
123-
OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS: 60
122+
# The maximum time for making requests to OTP.
123+
OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS: 60
124124

125125
# Use multi-threading to handle OTP requests and responses.
126126
OTP_REQUESTS_THREADING_ENABLED: "true"

Diff for: src/main/java/org/opentripplanner/middleware/models/ItineraryExistence.java

+4-7
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
import java.util.function.Function;
3636
import java.util.stream.Collectors;
3737

38-
import static org.opentripplanner.middleware.utils.ConfigUtils.getConfigPropertyAsInt;
38+
import static org.opentripplanner.middleware.otp.OtpDispatcher.OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS;
3939
import static org.opentripplanner.middleware.utils.ConfigUtils.getConfigPropertyAsText;
4040
import static org.opentripplanner.middleware.utils.DateTimeUtils.DEFAULT_DATE_FORMAT_PATTERN;
4141

@@ -46,9 +46,6 @@
4646
*/
4747
public class ItineraryExistence extends Model {
4848
private static final Logger LOG = LoggerFactory.getLogger(ItineraryExistence.class);
49-
private static final int OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS = getConfigPropertyAsInt(
50-
"OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS", 60
51-
);
5249
private static final String OTP_REQUESTS_THREADING_ENABLED = getConfigPropertyAsText(
5350
"OTP_REQUESTS_THREADING_ENABLED", "true"
5451
);
@@ -306,10 +303,10 @@ private Map<DayOfWeek, OtpResponse> getOtpResponses(List<OtpRequest> otpRequests
306303

307304
executor.shutdown();
308305
try {
309-
if (!executor.awaitTermination(OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
306+
if (!executor.awaitTermination(OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS, TimeUnit.SECONDS)) {
310307
LOG.warn(
311308
"OTP requests terminated, time out reached ({} seconds). Shutting down executor.",
312-
OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS
309+
OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS
313310
);
314311
executor.shutdownNow();
315312
}
@@ -321,7 +318,7 @@ private Map<DayOfWeek, OtpResponse> getOtpResponses(List<OtpRequest> otpRequests
321318
}
322319

323320
/**
324-
* Assign an OTP request to a day of the week.
321+
* Assign an OTP request to a day of the week and start each call async to the OTP server.
325322
*/
326323
private ConcurrentMap<DayOfWeek, CompletableFuture<OtpResponse>> assignOtpRequestToDayOfWeek(
327324
List<OtpRequest> otpRequests,

Diff for: src/main/java/org/opentripplanner/middleware/otp/OtpDispatcher.java

+4-1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import java.net.URI;
1818
import java.util.function.Supplier;
1919

20+
import static org.opentripplanner.middleware.utils.ConfigUtils.getConfigPropertyAsInt;
2021
import static org.opentripplanner.middleware.utils.ConfigUtils.getConfigPropertyAsText;
2122

2223
/**
@@ -37,7 +38,9 @@ public class OtpDispatcher {
3738
* Match the OTP GraphQL request timeout defined at
3839
* https://github.com/opentripplanner/OpenTripPlanner/blob/176e5f51923e82f8a4c2aa2a0b8284e1497b4439/src/main/java/org/opentripplanner/apis/gtfs/GtfsGraphQLAPI.java#L54
3940
*/
40-
private static final int OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS = 30;
41+
public static final int OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS = getConfigPropertyAsInt(
42+
"OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS", 30
43+
);
4144

4245
/**
4346
* Provides a response from the OTP server target service based on the query parameters provided.

Diff for: src/main/resources/env.schema.json

+5-5
Original file line numberDiff line numberDiff line change
@@ -224,16 +224,16 @@
224224
"examples": ["http://otp-server.example.com/otp"],
225225
"description": "The URL of an operational OTP (v2.x) server."
226226
},
227-
"OTP_REQUESTS_TERMINATION_TIMEOUT_SECONDS": {
228-
"type": "integer",
229-
"examples": ["60"],
230-
"description": "The maximum time for making requests to OTP as part of itinerary existence checks."
231-
},
232227
"OTP_REQUESTS_THREADING_ENABLED": {
233228
"type": "string",
234229
"examples": ["true"],
235230
"description": "Use multi-threading to handle OTP requests and responses."
236231
},
232+
"OTP_SERVER_REQUEST_TIMEOUT_IN_SECONDS": {
233+
"type": "integer",
234+
"examples": ["30"],
235+
"description": "The maximum time for making requests to OTP."
236+
},
237237
"OTP_TIMEZONE": {
238238
"type": "string",
239239
"examples": ["America/Los_Angeles"],

Diff for: src/test/java/org/opentripplanner/middleware/controllers/api/ApiUserFlowTest.java

-2
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,6 @@ void canSimulateApiUserFlow() throws Exception {
221221

222222
// After POST is complete, reset OTP response provider to default.
223223
ItineraryExistence.otpResponseProviderOverride = null;
224-
// Make sure all mocks were used
225-
assertTrue(mockResponses.areAllMocksUsed());
226224

227225
String responseBody = createTripResponseAsApiUser.responseBody;
228226
assertEquals(HttpStatus.OK_200, createTripResponseAsApiUser.status);

Diff for: src/test/java/org/opentripplanner/middleware/utils/ItineraryUtilsTest.java

-3
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,6 @@ void canCheckAllItinerariesExist(boolean insertInvalidDay, String message) throw
150150
// Days not monitored had an error response, so the check should return invalid for those days.
151151
assertFalse(existence.wednesday.isValid());
152152
assertFalse(existence.friday.isValid());
153-
154-
// Make sure all mocks were used
155-
assertTrue(mockResponses.areAllMocksUsed());
156153
}
157154

158155
private static Stream<Arguments> createCheckAllItinerariesExistTestCases() {

Diff for: src/test/java/org/opentripplanner/middleware/utils/MockOtpResponseProvider.java

-6
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,13 @@
1010
* Provides a set of mock OTP responses in the order they are expected to be used.
1111
*/
1212
public class MockOtpResponseProvider {
13-
private int index = 0;
1413
private final Map<DayOfWeek, OtpResponse> mockResponses;
1514

1615
public MockOtpResponseProvider(Map<DayOfWeek, OtpResponse> mockResponses) {
1716
this.mockResponses = mockResponses;
1817
}
1918

2019
public OtpResponse getMockResponse(OtpRequest otpRequest) {
21-
index++;
2220
return mockResponses.get(otpRequest.dateTime.getDayOfWeek());
2321
}
24-
25-
public boolean areAllMocksUsed() {
26-
return index == mockResponses.size();
27-
}
2822
}

0 commit comments

Comments
 (0)