Cluster consolidated stops in geocoder

[leonardehrenfried]

Summary

This connects two sandbox features with each other: IBI's consolidated stops and and the geocoder.

In order to do this, the LuceneIndex was moved out of the Graph and into a Dagger module which instantiates it (or not) based on the feature flags.

cc @miles-grant-ibigroup

Unit tests

I added a unit test.

Most importantly, it adds a dependency on the assertion library truth by Google which allows you to write assertions against collections (among other things) which have very helpful error messages, like this:

value of           : generateStopClusters(...)
expected to contain: LuceneStopCluster[primaryId=F:A, secondaryIds=[F:B], names=[A, B], codes=[A], coordinate=Coordinate[lat=60.0, lon=10.0]]
but was            : [LuceneStopCluster[primaryId=F:C, secondaryIds=[], names=[C], codes=[C], coordinate=Coordinate[lat=60.0, lon=10.0]], LuceneStopCluster[primaryId=F:A, secondaryIds=[F:B], names=[A, B], codes=[A, B], coordinate=Coordinate[lat=60.0, lon=10.0]]]

[codecov]

Codecov Report

Attention: Patch coverage is 75.47170% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.47%. Comparing base (b9e54d2) to head (9b17f97).
Report is 42 commits behind head on dev-2.x.

❗ Current head 9b17f97 differs from pull request most recent head f95fd60

Please upload reports for the commit f95fd60 to get more accurate results.

Files	Patch %	Lines
...opentripplanner/ext/geocoder/GeocoderResource.java	0.00%	4 Missing ⚠️
...planner/ext/geocoder/configure/GeocoderModule.java	0.00%	4 Missing ⚠️
.../org/opentripplanner/ext/geocoder/LuceneIndex.java	60.00%	2 Missing ⚠️
...ner/standalone/configure/ConstructApplication.java	0.00%	2 Missing ⚠️
...standalone/server/DefaultServerRequestContext.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5907      +/-   ##
=============================================
+ Coverage      69.45%   69.47%   +0.01%     
- Complexity     17064    17082      +18     
=============================================
  Files           1927     1932       +5     
  Lines          73578    73615      +37     
  Branches        7549     7550       +1     
=============================================
+ Hits           51106    51146      +40     
+ Misses         19847    19845       -2     
+ Partials        2625     2624       -1     

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

[t2gran]

I looked at Truth vs AssertJ. They are similar. AssertJ is a bit more open, but Truth is probably a bit more well designed.

This is written by the people creating Truth, but seems to be fairly objective and the best comparison I could find. I also looked at a few key numbers on the github repo (stars/users/activity) and they both seems to be maintained and mature.

Conclusion: I like Truth slightly better than AssertJ. I support starting to experiment with Truth (the core lib only) and decide later if we want to keep it or not. This do add "another lib" witch a OTP developer should master. There are pitfalls, so it everyone doing reviews need to spend time on learning it, if we decide to go "all in".

[binh-dam-ibigroup]

Works on clustered bus stops in Atlanta as far as I have tested. Just one comment typo but otherwise it looks solid.

[t2gran]

If I got it right(not sure if I am), then the LuceneIndex should not be request-scoped (building the index for revery request). I do not think it need to depend on the TransitService, it could use only the stop-model?

[t2gran]

The factory create a new instance every time you call it? I do not think this code has any effect?

[leonardehrenfried]

The LuceneIndex is annotated as @Singleton:

OpenTripPlanner/src/ext/java/org/opentripplanner/ext/geocoder/configure/GeocoderModule.java

Line 19 in a714a53

@Singleton

The idea here is to initialize the geocoder eagerly rather than on the first request.

[t2gran]

I think this is creating the index - reindexing for every request?

[t2gran]

You should pass it in using the builder if you do not want it to be created every time.

[t2gran]

The transit service is request scoped, is the LuceneIndex request-scoped?

[leonardehrenfried]

No, it isn't and I've reworked this in the newest commits. However, I do need a few methods from TransitService. Is it ok to instantiate it myself or should I create my own wrapper around the TransitModel?

[leonardehrenfried]

Note to self: add comment to LuceneIndex constructor.

[t2gran]

I will come back to this after doing the DI on the API resources.