Skip to content

Move debug timing to RoutingWorker and adapt to OTP2 routing phases - #3109

Merged
hannesj merged 6 commits into
opentripplanner:dev-2.xfrom
CGI-Move:debug-output
Aug 4, 2020
Merged

hannesj merged 6 commits into
opentripplanner:dev-2.xfrom
CGI-Move:debug-output

Conversation

@hannesj

@hannesj hannesj commented Jun 23, 2020

Copy link
Copy Markdown
Contributor

This makes DebugOutput use the new routing phases in OTP2, and updates their calculation. Previously a time of 0ms was reported for everything, as the DebugOutput object was not used in the routing worker.

To be completed by pull request submitter:

  • issue: Link to or create an issue that describes the relevant feature or bug. Add GitHub keywords to this PR's description (e.g., closes #45).
  • roadmap: Check the roadmap for this feature or bug. If it is not already on the roadmap, PLC will discuss as part of the review process.
  • tests: Have you added relevant test coverage? Are all the tests passing on the continuous integration service (Travis CI)?
  • formatting: Have you followed the suggested code style?
  • documentation: If you are adding a new configuration option, have you added an explanation to the configuration documentation tables and sections?
  • changelog: add a bullet point to the changelog file with description and link to the linked issue

To be completed by @opentripplanner/plc:

  • reviews and approvals by 2 members, ideally from different organizations
  • after merging: update the relevant card on the roadmap

@hannesj
hannesj requested a review from a team June 23, 2020 07:30
@hannesj
hannesj force-pushed the debug-output branch 2 times, most recently from 2a5a93c to 8c1e665 Compare June 23, 2020 07:43
@t2gran t2gran added the OTP2 label Jun 23, 2020
@hannesj hannesj changed the title Move DebugOutput timing to RoutingWorker and adapt to OTP2 routing phases Move debug timing to RoutingWorker and adapt to OTP2 routing phases Jun 23, 2020

@t2gran t2gran left a comment •

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.

My main comment here is that the DebugOutput plays two roles and should be split into a plan DTO
located in the org.opentripplanner.api.resource and a sevice witch do the timing and aggregates the results , and in the end produces the DTO. Maybe DebugAggregatoris an ok name for the service in lack of a better one. I would put it in the org.opentripplanner.routing.framework package. I could not find an existing good candidate.

If we do so, we get one LOGGER we can turn on/off for perfomance logging and the concerns are separated.

Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
t2gran
t2gran previously approved these changes Jul 30, 2020

@t2gran t2gran left a comment

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.

This looks as a good start. JavaDoc on some public classes is missing.

@hannesj
hannesj merged commit e54dcc9 into opentripplanner:dev-2.x Aug 4, 2020
@hannesj
hannesj deleted the debug-output branch August 4, 2020 20:00
@abyrd abyrd added this to the 2.0 milestone Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants