Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make StopIndex (counter) part of StopModel - not a global static instance #5488

Merged
merged 18 commits into from
Nov 9, 2023

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Nov 7, 2023

Summary

The StopModel should be responsible for the consistency and integrity of all stop related information. If two StopModels exist, then the stop index for both should start at 0 and increase by one for each RegularStop, it is ok with gaps, butmany gaps would lead to poor performance.

Issue

Closes #5480

There are tests witch relay on resetting the global stop-index, these test fail from time to time depending on the order thay are executed.

Design and alternative solutions

The stop model is prepared for injecting a context into the builders - this is something we have planed for, and wanted to do as part of the Transmodel refacoring. So, this PR then become a POK on this - we might change the design later, but we something like this. It is easier to do this on the StopModel, than on the rest of the TransitModel, because the StopModel do not have real-time updates. This is is complex solution, but it allows us to do different things:

  • Let the model be in charge of the integrity of the model. When something is added, deleted or updated the model can maintain indexes and other relations while the caller do not have to remember to notify the model. The model is its own master. We can also add events to this and let other models subscribe to changes, so they can update them self. The stop model is currently "static", so I will use another model as an example. Lets use a TripPattern in the TransitModel as an example. When a new trip pattern is added, we could notify the TransferModel and the transfer model can check if any new stops are visited - if so, it will generate new transfers. The same code will be run either as part of real-time updates or during graph build.

Pros/cons: The design is flexible and let us encapsulates each model. Making it easy to test, scale and maintain. It is a bit complex to understand, and if the events between each model is not done right - it become very difficult to debug. So, we want good traceability for events and some kind of "markers/commits" to signal when to publish real-time updates. We will use this with the current copy-on-write strategy. A model is replaced with a new one - allowing trip requests to query a consistent set of models (through the Service interfaces).

Alternatives: In stead of injecting the model into builder we can explicit add entities to the model after a set of entities are created. This is a more common approach, but does not change much - except that it limits the models influence over the entity creation.

Unit tests

This PR refactor many tests, but do not add any new tests.

Documentation

JavaDoc should be updated.

Changelog

No.

Bumping the serialization version id

Yes.

@t2gran t2gran added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog labels Nov 7, 2023
@t2gran t2gran added this to the 2.5 (next release) milestone Nov 7, 2023
@leonardehrenfried
Copy link
Member

I'm available to help out if you need me.

@t2gran t2gran force-pushed the otp2_fix_stop_index branch from a5bd204 to 3089724 Compare November 9, 2023 09:51
@t2gran t2gran marked this pull request as ready for review November 9, 2023 09:51
@t2gran t2gran requested a review from a team as a code owner November 9, 2023 09:51
@leonardehrenfried
Copy link
Member

Can you resolve the conflicts?

@t2gran t2gran force-pushed the otp2_fix_stop_index branch from 3089724 to c0a4198 Compare November 9, 2023 10:02
@t2gran
Copy link
Member Author

t2gran commented Nov 9, 2023

I need to get Java 21 locally, before fixing the merge.

I planed to inject a context into the builders, so I have more code that I did not commit - I am not to happy with the design - the generics look ugly. And also, I run into a problem with the RegularStop#copy() - I will deprecate the copy method and provide a proper alternative - but I can do that in another PR - we should express review this one, I got 2 merge conflicts just from yesterday to today...

@leonardehrenfried
Copy link
Member

I can also fix the conflicts but it's a bit easier when it's on a branch that I can push to. How about I push this to an branch in the opentripplanner repo which we then merge back to this one?

@t2gran t2gran force-pushed the otp2_fix_stop_index branch from c0a4198 to ebe7083 Compare November 9, 2023 10:24
@t2gran
Copy link
Member Author

t2gran commented Nov 9, 2023

The PR is ready to review now, most of the changes is in tests. If you want to review this together we can shedule a meeting - that is the most effective, I can ask @vpaturet to join and hopefully we can merge it. I will move it to OTP Repo so you can edit it directy as well.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (ec55557) 66.84% compared to head (a316ba0) 66.85%.
Report is 8 commits behind head on dev-2.x.

Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #5488   +/-   ##
==========================================
  Coverage      66.84%   66.85%           
- Complexity     15633    15650   +17     
==========================================
  Files           1816     1817    +1     
  Lines          70084    70155   +71     
  Branches        7376     7378    +2     
==========================================
+ Hits           46848    46899   +51     
- Misses         20779    20798   +19     
- Partials        2457     2458    +1     
Files Coverage Δ
...ner/ext/siri/updater/azure/SiriAzureETUpdater.java 0.00% <ø> (ø)
...builder/module/AddTransitModelEntitiesToGraph.java 59.50% <ø> (ø)
.../opentripplanner/gtfs/graphbuilder/GtfsModule.java 69.63% <100.00%> (+0.15%) ⬆️
...er/gtfs/mapping/GTFSToOtpTransitServiceMapper.java 95.29% <100.00%> (+0.05%) ⬆️
...g/opentripplanner/gtfs/mapping/LocationMapper.java 85.71% <100.00%> (+1.50%) ⬆️
...ntripplanner/model/impl/OtpTransitServiceImpl.java 94.00% <100.00%> (ø)
...in/java/org/opentripplanner/netex/NetexBundle.java 93.10% <100.00%> (ø)
...pentripplanner/netex/configure/NetexConfigure.java 100.00% <100.00%> (ø)
...opentripplanner/netex/mapping/FlexStopsMapper.java 86.48% <100.00%> (+0.18%) ⬆️
...ipplanner/netex/mapping/GroupOfStationsMapper.java 13.95% <ø> (ø)
... and 29 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I have left a few comments that you should take a look at but to me this looks ready for merging.

Co-authored-by: Leonard Ehrenfried <[email protected]>
@t2gran
Copy link
Member Author

t2gran commented Nov 9, 2023

✅ tested: street graph -> ser -> transit graph -> ser -> routing (Entur data, whole of Norway)

@t2gran t2gran requested a review from vpaturet November 9, 2023 13:41
@leonardehrenfried
Copy link
Member

I also tested this branch with a German and an American data set. There was no problem.

@leonardehrenfried leonardehrenfried merged commit 2429c5e into opentripplanner:dev-2.x Nov 9, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Nov 9, 2023
@t2gran t2gran deleted the otp2_fix_stop_index branch November 16, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog Technical Debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global variable StopLocation.INDEX_COUNTER causes race conditions during tests
3 participants