Siri file loader #5460
Siri file loader #5460
Conversation
…, ZonedDateTime time)
In a busy updater it does not matter if we commit early or late, as long as we throttle the commit to happen with a given frequency. But, when testing OTP manually, it is easy to do mistakes and assume that an update is applied when you see "INFO 1 of 1 update messages were applied successfully (success rate: 100.0%)". In OTP before this commit, the update was not committed, waiting to fill up the "buffer", and if not other update was applied - nothing happened.
This feature make it much simpler to test realtime updates.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5460 +/- ##
=============================================
- Coverage 66.87% 66.85% -0.03%
Complexity 15504 15504
=============================================
Files 1800 1802 +2
Lines 69817 69856 +39
Branches 7359 7359
=============================================
+ Hits 46692 46699 +7
- Misses 20666 20697 +31
- Partials 2459 2460 +1
☔ View full report in Codecov by Sentry. |
...in/java/org/opentripplanner/standalone/config/routerconfig/updaters/SiriSXUpdaterConfig.java
Outdated
Show resolved
Hide resolved
...in/java/org/opentripplanner/standalone/config/routerconfig/updaters/SiriSXUpdaterConfig.java
Outdated
Show resolved
Hide resolved
src/ext/java/org/opentripplanner/ext/siri/updater/SiriFileLoader.java
Outdated
Show resolved
Hide resolved
src/ext/java/org/opentripplanner/ext/siri/updater/SiriFileLoader.java
Outdated
Show resolved
Hide resolved
| Siri updates = updateSource.getUpdates(); | ||
| if (updates != null) { | ||
| var updates = updateSource.getUpdates(); | ||
| if (updates.isPresent()) { |
There was a problem hiding this comment.
While I think that Optional is great, I'm not a huge fan of the combination of isPresent and get.
I think you should restructure the code a bit so that you can use map() instead. If you agree that this is the way to go, you can cherry-pick this commit: leonardehrenfried@5e8f239
There was a problem hiding this comment.
I am sorry I don´t find your suggestion readable. Big blocks of code inside a call chain is not recommended. Second, the block is also not a function - it updates the state of the class.
| long t1 = System.currentTimeMillis(); | ||
| try { | ||
| Siri siri = siriHttpLoader.fetchSXFeed(requestorRef); | ||
| Optional<Siri> siri = siriHttpLoader.fetchSXFeed(requestorRef); |
There was a problem hiding this comment.
Again, I would use flatMap() here which saves you dealing with the empty case: leonardehrenfried@873b5b7
There was a problem hiding this comment.
Introducing the flatMap is mentally much harder than to step through the logic and return in each case. I think the fact that your code does not work - and that the error is hard to spot proves my point.
There is a a few advanced programming techniques I personally would love to us in OTP - but I don´t, because we struggle to even follow the basic ones (encapsulation, single-responsibility, same-level-of-abstraction, DRY ...). I try to stick to the what is already use. OO programming techniques are very important in OTP because it matches the problem we are trying to solve. Functional programming is a nice tool, but it does not go well with the imperative code we have (we do agree on removing the imperative code). For example I do not want to pass Optional into methods, and we have a lot passing data down a deep call stack. So, it the return value is passed on, not acted on, then it does not make sense to return an Optional.
Using map to avoid dealing with empty might be a trick in functional programming - but it does not clearly show the intent, and to me looks more like a hack. I would love to use more Domain Driven Design, and then you would not use Optional type at all - you would create your own types witch uses the domain language to clearly state the intent.
src/main/java/org/opentripplanner/framework/time/CountdownTimer.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
|
You need to regenerate the docs. |
Summary
Add support for loading xml files for Siri SX and EX updates. This feature make it much simpler to test realtime updates. This PR also has a few minor improvements and code cleanup.
Always commit the first update after a long break
In a busy updater it does not matter if we commit early or late, as long as we throttle the update commits to happen with a given frequency. But, when testing OTP manually, it is easy to do mistakes and assume that an update is applied when you
see (log):
In OTP before this commit, the update was not committed, waiting to fill up the "buffer", and if not other update was applied - nothing happened.
File loader
Use the
urlto set a directory. The updater will use the directory instead of connecting to a http resource. The file loader will look for xml files:*.xmlin the configured directory. The files are renamed by the loader when possessed:Issue
No.
Unit tests
No.
Documentation
✅
Changelog
No
Bumping the serialization version id
No