Add trip handling strategies for MTC feed merge by landonreed · Pull Request #376 · ibi-group/datatools-server

landonreed · 2021-04-29T15:58:49Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR implements changes to MTC's service period merge feature as defined here: https://docs.google.com/document/d/1mx9s3XfWfPB7SSXDwCbnRF7F2zLDU2ED4gK-W6eXL9o/edit

This PR also fixes #244.

Notable changes include:

Moving a bunch of pre-existing things (FeedToMerge class and util methods) to new files to clean things up.
Implementing MergeStrategy enum to mimic strategies in google doc.
Update to gtfs-lib in order to get the StopTime#hashCode method.
Some more tests.
Note: Works with Mtc merge 2021 tweaks datatools-ui#686

src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsResult.java

evansiroky · 2021-04-29T16:53:35Z

src/main/java/com/conveyal/datatools/manager/jobs/MergeStrategy.java

+    /**
+     * If service_ids and trip_ids in active feed are the same as future feed then the service end date for the
+     * merged feed shall match with future feed’s service end date and the service start date for the merged feed
+     * should be the merged date. All files from the future feed only shall be used in the merged feed.


It is unclear from this comment what "merged date" is. If a reference to either the active or future feed's dates could be made, that would make it explicit.

It sounds like the "reconciled" start and end date of the entry in the calendar table, but I agree that rewording this term would help.

evansiroky · 2021-04-29T16:55:32Z

src/main/java/com/conveyal/datatools/manager/jobs/MergeStrategy.java

+    /**
+     * If service_ids in active and future feed exactly match but only some of the trip_ids match then the merge
+     * strategy shall handle the following three cases:
+     * - *trip_id in both feeds*: The service shall start from the data merge date and end at the future feed’s service


Can you be more explicit on what the data merge date would be here?

evansiroky · 2021-04-29T16:57:43Z

src/main/java/com/conveyal/datatools/manager/jobs/MergeStrategy.java

+     * - *trip_id in past feed*: A new service shall be created starting from the merge date and expiring at the end
+     *   of active service period.


Please clarify what "merge date" is and it'd be good to stick with the same terminology for past/active/future feed. Past is always active, right?

src/main/java/com/conveyal/datatools/manager/jobs/MergeStrategy.java

evansiroky · 2021-04-29T20:50:06Z

src/main/java/com/conveyal/datatools/manager/utils/MergeFeedUtils.java

+        CsvReader csvReader = table.getCsvReader(zipFile, null);
+        if (csvReader == null) {
+            LOG.warn("Table {} not found in zip file: {}", table.name, zipFile.getName());
+            return ids;
+        }
+        Field[] fieldsFoundInZip = table.getFieldsFromFieldHeaders(csvReader.getHeaders(), null);
+        // Get the key field (id value) for each row.
+        int keyFieldIndex = getFieldIndex(fieldsFoundInZip, keyField);
+        while (csvReader.readRecord()) ids.add(csvReader.get(keyFieldIndex));
+        csvReader.close();


Please ensure this CsvReader is always closed. Maybe use a try-with-resources block.

I don't think try with resources can be used because the class does not implement AutoCloseable. Are there scenarios you've seen where this is not getting closed?

If there's any exception that occurs before the reader is closed, then the reader will be left open. You're right about CsvReader not implementing Closeable, so better wrap all this with try-finally.

Looks like this still needs to be implemented.

Handled in 06b2156.

src/main/java/com/conveyal/datatools/manager/utils/MergeFeedUtils.java

evansiroky · 2021-04-29T21:12:59Z

src/test/java/com/conveyal/datatools/manager/jobs/MergeFeedsJobTest.java

        );
    }

+//    /**


Can you add a comment for why there is a big comment block here?

evansiroky · 2021-04-29T21:19:28Z

src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java

+        pastServiceIds.addAll(currentFeed.idsForTable.get(Table.CALENDAR));
+        pastServiceIds.addAll(currentFeed.idsForTable.get(Table.CALENDAR_DATES));


Is it past or current. It'd be really helpful to pick one thing to call this thing to avoid confusion.

src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java

binh-dam-ibigroup

Please see the formatting and refactoring suggestions for now.

binh-dam-ibigroup · 2021-04-29T20:09:50Z

src/main/java/com/conveyal/datatools/manager/jobs/MergeStrategy.java

+    /**
+     * If service_ids and trip_ids in active feed are the same as future feed then the service end date for the
+     * merged feed shall match with future feed’s service end date and the service start date for the merged feed
+     * should be the merged date. All files from the future feed only shall be used in the merged feed.


It sounds like the "reconciled" start and end date of the entry in the calendar table, but I agree that rewording this term would help.

binh-dam-ibigroup · 2021-04-29T20:16:36Z

src/main/java/com/conveyal/datatools/manager/utils/MergeFeedUtils.java

+        Field[] fieldsFoundInZip = table.getFieldsFromFieldHeaders(csvReader.getHeaders(), null);
+        // Get the key field (id value) for each row.
+        int keyFieldIndex = getFieldIndex(fieldsFoundInZip, keyField);
+        while (csvReader.readRecord()) ids.add(csvReader.get(keyFieldIndex));


Put the body of the while loop on a separate line and in brackets, please.

src/main/java/com/conveyal/datatools/manager/utils/MergeFeedUtils.java

src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java

src/test/java/com/conveyal/datatools/manager/jobs/MergeFeedsJobTest.java

src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java

evansiroky

This MergeFeedsJob is becoming really complex to the point where it's kind of unclear what all is going on without doing a deep dive into it. To help with this, each step should be thoroughly commented and also, I think it'd really help to have consistent terminology about what past/current/future feeds are.

…dates.

…alendar_attributes.

…trips table.

…fixes.

…tial table write.

binh-dam-ibigroup

Approving following client approval.

fix(MergeFeedsJob): add trip handling strategies for MTC merges

0111293

landonreed changed the title ~~fix(MergeFeedsJob): add trip handling strategies for MTC merges~~ Add trip handling strategies for MTC feed merge Apr 29, 2021

landonreed assigned evansiroky and binh-dam-ibigroup Apr 29, 2021

landonreed mentioned this pull request Apr 29, 2021

Add hashCode and equals method to StopTime.java conveyal/gtfs-lib#315

Merged

5 tasks