Skip to content

Adjust search window dynamically to fit the number of itineraries - #3828

Merged
t2gran merged 40 commits into
dev-2.xfrom
otp2_inc_search_window_in_sub_seq_searches
Feb 4, 2022
Merged

t2gran merged 40 commits into
dev-2.xfrom
otp2_inc_search_window_in_sub_seq_searches

Conversation

@t2gran

@t2gran t2gran commented Jan 25, 2022

Copy link
Copy Markdown
Member

Summary

If the result have too few or too many itineraries we adjust the search-window for the next request to better fit the expected size of the search-window. This is not done for the initial search, but for all searches after that. The search-window will scale down to fit the numOfItineraries asked for, or up to 6 itineraries in the result. When paging from a high frequency

In addition this PR also clean up the Paging functionality and add documentation for this.

fix: Calculate start of search window correct

We need to include an extra minute when we want to include the "first removed itinerary" start time in the search window. The search window is [inclusive, exclusive] so to include a time t at the end the search window need to be [t0 , t+1m].

Issue

There is no issue for this.

Unit tests

New unit tests are added and old updated.

Code style

Documentation

✅ This add design doc for paging

Changelog

t2gran added 20 commits January 17, 2022 14:50
Extract duplicate code and generate the previous and next token together.
Extract duplicate code and generate the previous and next token together.
The filter chain is responsible for sorting and cropping the itinerary list is the maxNumOfItineraries is
reached. Before this commit these features were tied to the request#arriveBy. By removing this coupling
we can later use the sort-order and cropping independently of what the arriveBy is, witch is needed for
pageing to work properly.
We will add doc and several new classes to this package later.
Refactor the PageCursorFactoryTest by removing boilerplate and
update test names to match design doc.
This will make code easier to read and later changes
easier to diff and compare with the design doc.
The 'transitTimeZero' is the base for all scheduled times for this request/search.
It is usually at midnight for the given request 'dateTime' and all schedule times
are relative to this point in time.
We need to include an extra minute when using the last minute
to be included in the seach window to calculate the start of it.
If the result have too few or too many itineraries we adjust the
search-window for the next request to better fit the expected
size of the search-window.
@t2gran t2gran added this to the 2.1 milestone Jan 25, 2022
@t2gran
t2gran requested a review from a team as a code owner January 25, 2022 11:38
Comment thread src/main/java/org/opentripplanner/model/plan/pagecursor/PageCursorFactory.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
Comment thread src/main/java/org/opentripplanner/model/plan/SearchWindowUtils.java Outdated
Comment thread src/main/java/org/opentripplanner/model/plan/SearchWindowUtils.java Outdated
Comment thread src/main/resources/logback.xml Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
hannesj
hannesj previously approved these changes Feb 1, 2022

@hannesj hannesj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@t2gran

t2gran commented Feb 2, 2022

Copy link
Copy Markdown
Member Author

What should be the relation between the user set search time window, min/maxWinTimeMinutes and this automatic search window adjusting? I feel like it might make sense that the search window defined by the user is not respected in the next/previous pages but that should be documented. However, I feel like we should have a clear way to limit the maximum search window that can be achieved through the original time window and the adjustments by the paging logic.

I have addressed most of this in comments/fixes above, but I will try to give a summary here:

  • When paging we may change any request parameter, if we do we include it in the cursor and ignore the one in the request. We still ask the client to pass in the original request. We may change this without notifying the clients. So, the question is, if we need to document the details (fields overridden), or just the contract. We do return the searchWindowUsed for debugging purposes, so I have added doc for it - but not for other parameter witch is overridden (dateTime and arriveBy). The token i obfuscated using Base64 - a strong message that you should not use the information in it, but you can inspect it.
  • The tuning parameters min/maxWinTimeMinutes in the dynamic should probably be respected for all searches. I use it now to limit the window in the paging requests, but I do not think we use them to limit a request from the client with the searchWindow set. I think we should rename these config parameters as well. There is a bit of duplicated code now in the RaptorSearchWindowCalculator and the PagingFactory. This can probably be cleaned up by creating an interface (Role) in raptor and make RaptorSearchWindowCalculator use the functionality in the internal OTP Model.

Comment thread src/main/java/org/opentripplanner/model/plan/pagecursor/PageCursor.java Outdated
* arriveBy is true)
*/
private long dateTime = new Date().getTime() / 1000;
private Instant dateTime = Instant.now();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the Instant is a wrapper around epocSeconds, and getting rid of all the slightly different accessors for dataTime felt good. I see now that I forgot to update that JavaDoc, but I think I will draw the line here - there are 95% technical improvements in this PR...

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
hannesj
hannesj previously approved these changes Feb 3, 2022
@t2gran
t2gran dismissed stale reviews from hannesj and leonardehrenfried via 196b80a February 3, 2022 21:30
@t2gran

t2gran commented Feb 3, 2022

Copy link
Copy Markdown
Member Author

I added the possibility to configure the maxJourneyDuration witch we need at Entur. This is default 24H - this is a change from the existing 3 days, so make sure you add this to your configs if you need it!

This files are changed:

  • NodeAdapter.java
  • NodeAdapterTest.java
  • RoutingResponseMapper.java

@t2gran t2gran added !Bug Apply to issues describing a bug and PRs witch fixes it. !New Feature A functional feature targeting the end user. !Technical Debt Improve code quality, no functional changes. labels Feb 3, 2022
@leonardehrenfried

leonardehrenfried commented Feb 4, 2022

Copy link
Copy Markdown
Member

This looks really good. Do you want to add a line to Configuration.md about maxJourneyTime?

@t2gran

t2gran commented Feb 4, 2022

Copy link
Copy Markdown
Member Author

I can do it as a separate PR maybe to get this merged, I do not remember if we document the request parameters.

@t2gran
t2gran merged commit f04f1c9 into dev-2.x Feb 4, 2022
@t2gran
t2gran deleted the otp2_inc_search_window_in_sub_seq_searches branch February 4, 2022 09:07
t2gran pushed a commit that referenced this pull request Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. Entur Test This is currently being tested at Entur !New Feature A functional feature targeting the end user. !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants