Send notifications for feed expirations #126

evansiroky · 2018-11-28T00:37:10Z

Implementation to resolve #121.

Will send a 1 week warning email and an email on the day of an expiration.

Also has some refactoring of commonly shared code and fixes #123.

Fixes #121 Fixes #123

landonreed

This looks great. Comment summary: some methods need javadoc and we might should consolidate a bunch of this scheduling logic in a single class.

landonreed · 2018-11-28T20:07:24Z

src/main/java/com/conveyal/datatools/common/utils/Utils.java

+import java.time.ZoneId;
+
+public class Utils {
+    public static ZoneId getTimezone(String tzid) {


This new method should have javadoc.

landonreed · 2018-11-28T20:10:19Z

src/main/java/com/conveyal/datatools/manager/DataManager.java

@@ -90,6 +94,8 @@
    public static Map<String, ScheduledFuture> autoFetchMap = new HashMap<>();
    // Scheduled executor that handles running scheduled jobs.
    public final static ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
+    // multimap for keeping track of notification expirations
+    public final static ListMultimap<FeedSource, Future> scheduledNotificationExpirations = ArrayListMultimap.create();


Can we just store the feed source's ID here instead of the feed source? Actually, I was thinking we might should just keep all scheduled tasks in a single spot (i.e., adapt the autoFetchMap into a general purpose data structure for these.

landonreed · 2018-11-28T20:11:15Z

src/main/java/com/conveyal/datatools/manager/DataManager.java

@@ -126,6 +132,8 @@ public static void main(String[] args) throws IOException {
            }
        }

+        scheduleFeedNotificationExpirations();


I would like to see this method generalized to include the project auto fetch scheduled tasks above.

landonreed · 2018-11-28T20:24:40Z

pom.xml

@@ -60,6 +60,7 @@
                <filtering>true</filtering>
                <includes>


What is the purpose of this filtering? I don't love the idea of having to explicitly include each item here. Are we excluding something? If so, could we just define an excludes filter?

Looks like I copy-pasted it all in when I added the server info api endpoint. Agree that filtering should be off here.

Just tried a whole bunch of different things and none of them worked, so I think this is actually necessary.

There should be comments here showing why we are filtering resources though. Filtering means replacing {keys} like this with variables defined by Maven (often in the POM). In resources/public/index.html I see <link href="https://${S3BUCKET}.s3.amazonaws.com/dist/index.css" rel="stylesheet"> so filtering makes sense here. But I don't think we want to be performing variable substitution on the Logback configuration or YAML config files.

landonreed · 2018-11-28T20:26:45Z

src/main/java/com/conveyal/datatools/manager/DataManager.java

@@ -159,10 +167,26 @@ static void initializeApplication(String[] args) throws IOException {
        feedBucket = getConfigPropertyAsText("application.data.gtfs_s3_bucket");
        bucketFolder = FeedStore.s3Prefix;

+        // create application gtfs folder if it doesn't already exist
+        new File(getConfigPropertyAsText("application.data.gtfs")).mkdirs();


The other approach would be to have the application exit if no folder exists here. I'm kind of inclined to do that unless there's a reason why this is better. Curious to hear your thoughts.

When I'm using the dev configuration it lists the gtfs directory in tmp which I guess is getting deleted every so often. Therefore, I added it in here so it will always be created if the folder doesn't exist. The current code results in the application noting an error on startup, but not exiting, but then once a webservice is called all kinds of errors happen. I think it'd be good default behavior to create this folder if it doesn't exist.

What if someone is using a shared config file and the path is set to /Users/landon/nested/dir/that/is/very/deeply/buried. Do we really want to create those directories on the server? If the application isn't properly failing fast, let's just add that behavior. We can also just change the default gtfs directory if that behavior is annoying.

Regarding creating supposedly random folders on the server, if it's what the user specified, why wouldn't datatools-server do that? I don't even know what goes in that folder in the first place. I actually like that the files are saved to a tmp directory so that they get wiped out locally so I don't have to periodically clean things up. I'm also not a fan of the server failing because I haven't done even more manual setup of things. It should just work.

It is arbitrary how a server reacts when it's misconfigured. It can fail fast or attempt to recover by creating missing directories. The important thing is that we choose a behavior and stick to it. I'm increasingly a fan of failing fast and showing clear error messages. Every error recovery code path is another thing that needs to be maintained and can have unexpected consequences, e.g. if someone misspells the intended path and we silently create it, or attempt to create it and it fails with a seemingly irrelevant permissions error.

landonreed · 2018-11-28T20:34:18Z

src/main/java/com/conveyal/datatools/manager/models/FeedSource.java

+        scheduledNotificationExpirations.removeAll(this);
+    }
+
+    public void scheduleExpirationNotifications () {


Needs javadoc explaining what this is.

landonreed · 2018-11-28T20:34:57Z

src/main/java/com/conveyal/datatools/manager/models/FeedSource.java

+            if (parentProject == null) {
+                // parent project has been deleted, but feed source/version have not
+                // abort the setting up of the notification and figure out why the database has been
+                // allowed to devolve to this state


Should we not log a warning or error here?

Logging an warning does seem like a good idea here.

landonreed · 2018-11-28T20:36:13Z

src/main/java/com/conveyal/datatools/manager/models/FeedSource.java

+            long expirationEpochSeconds = latest
+                .validationResult
+                .lastCalendarDate
+                .atTime(0, 0)


Is this time set to midnight? From a user experience perspective, this might not be ideal. Why don't we schedule them for like 9am instead?

9am is way too late in the day because some people could start working before that. Maybe 4am is best since it wouldn't usually conflict with other items running on the server. I think this would be a good thing to be one of the first things someone sees in their inbox perhaps along with a nightly status report / summary.

landonreed · 2018-11-28T22:47:03Z

src/main/java/com/conveyal/datatools/manager/models/FeedSource.java

@@ -538,4 +544,88 @@ public FeedSource clone () throws CloneNotSupportedException {
        return (FeedSource) super.clone();
    }

+    @Override


If we just stash the feed ID in the multimap, we wouldn't need this equality method.

landonreed · 2018-11-28T22:54:02Z

src/main/java/com/conveyal/datatools/manager/controllers/api/ProjectController.java

-            }catch(Exception e){
-                timezone = ZoneId.of("America/New_York");
-            }
+            ZoneId timezone = getTimezone(project.defaultTimeZone);


Perhaps this scheduleAutoFeedFetch and the FeedSource#scheduleExpirationNotifications (and the respective cancel methods) should all go in the same class. It makes me uncomfortable that this logic is getting spread out and I suspect there is some duplicated code that could be factored out. Maybe this could all go in the datatools.common.util package as ScheduleUtils? Some of the logic in DataManager.java might belong here also.

I suppose that make sense. I'll try to do that.

A few other things I saw: it looks like the logic to schedule auto-fetch of specific feed sources is missing in datatools-server. Also, it looks like we need to reconfigure project creation to automatically schedule fetches for a project upon creation in light of the changes in catalogueglobal/datatools-ui#41

Wait nvm, I see that project auto-fetch must be enabled for a feed to automatically be fetched.

landonreed · 2018-11-29T16:33:44Z

@evansiroky, one other comment I have: can you run the cancel notifications method when a feed source is deleted (i.e., within FeedSource#delete)? While you're at it, please do the same for cancelling auto fetch when a project is deleted. Thanks!

evansiroky · 2018-12-03T03:26:46Z

@landonreed The method for deleting a project will call the feedsource delete method which will call the feedversion delete method which now recalculates the feed expiration notifications, so your request for adding to the delete methods should be needed.

it appears to be needed after all

evansiroky · 2018-12-04T20:13:32Z

Requested changes implemented. Should be ready for review and/or approval.

…utility

…tions-ltr Tweaks to Feed Expiration Notification PR

landonreed · 2018-12-11T19:02:50Z

OK, #130 has been merged into this PR. I think we can call this complete now.

feat(feed): send notifications for feed expirations

48d5b0e

Fixes #121 Fixes #123

evansiroky assigned landonreed Nov 28, 2018

landonreed requested changes Nov 28, 2018

View reviewed changes

landonreed assigned evansiroky and unassigned landonreed Nov 28, 2018

evansiroky added 4 commits December 4, 2018 11:28

refactor(scheduler): create scheduler class and address PR comments

3aebf7c

Merge branch 'dev' into feed-expiration-notifications

8f997fe

refactor(pom): add back resource filtering

8113686

it appears to be needed after all

refactor(jobs): fix FetchProjectFeedsJob merge problem

ab7781f

evansiroky assigned landonreed and unassigned evansiroky Dec 4, 2018

landonreed and others added 5 commits December 4, 2018 17:37

refactor(feed-expiration): consolidate scheduling logic in Scheduler …

2c76cd0

…utility

refactor: address PR comments (style + log change)

1bc7cde

refactor(Scheduler): address PR comments

09b6535

refactor(scheduler): rename scheduleJob

465719f

Merge pull request #130 from catalogueglobal/feed-expiration-notifica…

33a4518

…tions-ltr Tweaks to Feed Expiration Notification PR

landonreed approved these changes Dec 11, 2018

View reviewed changes

landonreed merged commit 2d70c92 into dev Dec 11, 2018

landonreed deleted the feed-expiration-notifications branch December 11, 2018 19:03

This was referenced Dec 11, 2018

Send notifications for feed expirations ibi-group/datatools-server#145

Closed

Create project and feed refactor catalogueglobal/datatools-ui#41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send notifications for feed expirations #126

Send notifications for feed expirations #126

evansiroky commented Nov 28, 2018

landonreed left a comment

landonreed Nov 28, 2018

landonreed Nov 28, 2018

landonreed Nov 28, 2018

landonreed Nov 28, 2018

evansiroky Dec 4, 2018

evansiroky Dec 4, 2018

abyrd Dec 11, 2018

landonreed Nov 28, 2018

evansiroky Dec 3, 2018

landonreed Dec 4, 2018

evansiroky Dec 4, 2018

abyrd Dec 11, 2018

landonreed Nov 28, 2018

landonreed Nov 28, 2018

evansiroky Dec 3, 2018

landonreed Nov 28, 2018

evansiroky Dec 3, 2018

landonreed Nov 28, 2018

evansiroky Dec 3, 2018

landonreed Nov 28, 2018

evansiroky Dec 3, 2018

evansiroky Dec 3, 2018

landonreed commented Nov 29, 2018 •

edited

Loading

evansiroky commented Dec 3, 2018

evansiroky commented Dec 4, 2018

landonreed commented Dec 11, 2018

Send notifications for feed expirations #126

Send notifications for feed expirations #126

Conversation

evansiroky commented Nov 28, 2018

landonreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed commented Nov 29, 2018 • edited Loading

evansiroky commented Dec 3, 2018

evansiroky commented Dec 4, 2018

landonreed commented Dec 11, 2018

landonreed commented Nov 29, 2018 •

edited

Loading