Fixes #462, Integrate inactive blog url and include logic to filter inactive feeds #639

c3ho · 2020-02-02T00:09:40Z

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

As per discussion, removed the inactive-blog-filter.js files (sorry Rafi). Added new functions in storage.js + feed.js to handle invalid feeds. Modified processor.js to use the new functions for the appropriate errors.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

humphd

This is heading in the right direction. I left some initial comments.

src/backend/utils/storage.js

src/backend/data/feed.js

src/backend/utils/storage.js

src/backend/data/feed.js

src/backend/feed/processor.js

vercel · 2020-02-04T03:37:56Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/humphd/telescope/fqsooi59p
✅ Preview: https://telescope-git-fork-c3ho-issue-462.humphd.now.sh

c3ho · 2020-02-04T03:39:27Z

Still need to work on the logic and the failedReason, fail event has been added and can kind of confirm it is working.

Edit: There is no way to access the failedReason as noted by this issue here

c3ho · 2020-02-04T03:50:42Z

To test, the stuff for now:

set the .env var FEED_QUEUE_ATTEMPTS to 1 AND inside queue.js set removeOnFail: to removeOnFail: false<--- this prevents failed jobs from disappearing so you can use any of the ids there to test
npm start
check for failed jobs in localhost:(yourPort#Here)/admin/queues in the failed tab
open up a terminal to interact with redis and use the following command
redis-cli exists t:feed:jobId:invalid jobId being whatever id you see in the failed queue, if it returns 1, its there
to check the reason why it was invalid use:
redis-cli get t:feed:jobId:invalid

for example I have a job id #937dda54ec
redis-cli exists t:feed:937dda54ec:invalid should return 1
redis-cli get t:feed:937dda54ec:invalid should also give me a reason.

I set a logger.debug message to display something along the lines of Skipping (job.id) as it is an invalid feed. After the initial run the console should display something like that message pretty often

c3ho · 2020-02-04T08:08:15Z

Further testing is making it weird, its also saving the posts it can't process as part of the t:feed:**:invalid set to Redis. Not entirely sure how the err is supposed to behave as there's a discrepancy between the bull board failureReason and the reason in redis

src/backend/data/feed.js

src/backend/feed/processor.js

humphd · 2020-02-04T15:55:18Z

src/backend/feed/queue.js

@@ -26,7 +26,7 @@ queue.addFeed = async function(feed) {
      delay: process.env.FEED_QUEUE_DELAY_MS || 60 * 1000,
    },
    removeOnComplete: true,
-    removeOnFail: true,
+    removeOnFail: false,


Can you explain this change? Will this still allow the job to be retried?

Sorry, enabled that for now just so I can refer to which jobs failed or not and can manually find them in redis

humphd · 2020-02-04T16:06:10Z

src/backend/index.js

@@ -54,6 +54,10 @@ function processFeeds(feeds) {
      const currentFeed = await updateFeed(feed);
      // Add a job to the feed queue to process all of this feed's posts.
      await feedQueue.addFeed(currentFeed);
+      // If failed, set the feed to invalid and save to Redis.
+      feedQueue.on('failed', (job, err) => {
+        currentFeed.setInvalid(err.message);


When you have a callback in an event handler that is synchronous, and you need to invoke an async function, you need to add a .catch() so a failed Promise doesn't blow things up. Also, you shouldn't hold a reference to the feed in a closure like this. Use the job.data to recreate a new Feed object:

async function invalidateFeed(feedData) { const feed = Feed.parse(feedData); await feed.setInvalid(feedData.reason || 'unknown reason'); } feedQueue.on('failed', (job, err) => invalidateFeed(job.data).catch(error => logger.error({ error }, 'Unable to invalidate feed in database.')));

NOTE: we'll need to attach the reason to the job.data before we fail it. I'm not 100% sure where/how to do this without doing it myself.

I tried your above code, it does work... just like you said all of them have a value of 'unknown reason'

Another side effect of it being failed, I'm not sure if its a bug. Bull can't differentiate between feeds failing and posts failing, it adds both to Redis

test/storage.test.js

humphd · 2020-02-04T20:17:09Z

I think you need to rebase to get the code where I fix the posts not failing.

c3ho · 2020-02-04T20:53:00Z

Nice, that did the trick.

humphd

Nice. Amazing how compact this code is now. That's fantastic. A few more things.

src/backend/index.js

src/backend/utils/storage.js

humphd

One last thing, and let's take this. Great work.

src/backend/index.js

humphd

Let's try this, and fix bugs as we go.

c3ho · 2020-02-06T02:07:03Z

Testing this on windows dev environment, I didn't get all those errors popping up unlike the demo this morning when shutting down the service with ctrl + c

c3ho · 2020-02-07T04:15:37Z

Going to just merge this once it passes CI, since I'm just rebasing stuff.

humphd requested changes Feb 2, 2020

View reviewed changes

Silvyre self-requested a review February 3, 2020 15:11

c3ho force-pushed the issue-462 branch from 08a49bb to 8afb128 Compare February 4, 2020 03:35

vercel bot deployed to Preview February 4, 2020 03:36 View deployment

vercel bot deployed to Preview February 4, 2020 03:43 View deployment

c3ho force-pushed the issue-462 branch from 7b7b061 to 750e1d8 Compare February 4, 2020 04:56

vercel bot deployed to Preview February 4, 2020 04:56 View deployment

c3ho requested a review from humphd February 4, 2020 05:00

vercel bot deployed to Preview February 4, 2020 08:23 View deployment

humphd requested changes Feb 4, 2020

View reviewed changes

c3ho force-pushed the issue-462 branch from c6e9e0c to 233d090 Compare February 4, 2020 20:52

vercel bot deployed to Preview February 4, 2020 20:52 View deployment

vercel bot deployed to Preview February 4, 2020 21:03 View deployment

humphd requested changes Feb 4, 2020

View reviewed changes

src/backend/index.js Show resolved Hide resolved

src/backend/index.js Outdated Show resolved Hide resolved

src/backend/utils/storage.js Show resolved Hide resolved

vercel bot deployed to Preview February 5, 2020 03:49 View deployment

humphd requested changes Feb 5, 2020

View reviewed changes

src/backend/index.js Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview February 6, 2020 01:52 Inactive

vercel bot deployed to Preview February 6, 2020 01:54 View deployment

c3ho requested a review from humphd February 6, 2020 01:58

c3ho force-pushed the issue-462 branch from 6f14dde to 64dff87 Compare February 6, 2020 02:00

vercel bot deployed to Preview February 6, 2020 02:00 View deployment

humphd previously approved these changes Feb 6, 2020

View reviewed changes

c3ho linked an issue Feb 6, 2020 that may be closed by this pull request

Integrate inactive blog url and include logic to filter inactive feeds #462

Closed

c3ho added 22 commits February 6, 2020 23:14

updated process.js to include logic

f301048

fixed typo

1c84406

commented check for now

e8158c0

changes from DH feedback

35762f3

fixed processor.js

b01fa9c

added queue fail event

2a4149e

changed err to err.message for clarity

71c6943

updated storage.test.js code

e0739b0

more changes

8883f4f

removed feeds-redlist.json

b02dc3d

updated storage.test.js with a test

c0793fb

updated index.js

d6700ee

disabled removeOnFail

f21943f

other changes

97c49d3

some changes

9192106

removed new tests for now

df2b915

changes

b7e12df

removed tests for now

e166080

changed removeOnFail back to true

5532685

added logging info AND fixed documentation

ca2c196

failed event

74ef25b

changed comment

81be74c

c3ho dismissed stale reviews from cindyorangis, Silvyre, and humphd via 81be74c February 7, 2020 04:14

c3ho force-pushed the issue-462 branch from 64dff87 to 81be74c Compare February 7, 2020 04:14

vercel bot deployed to Preview February 7, 2020 04:14 View deployment

c3ho merged commit 11700c0 into Seneca-CDOT:master Feb 7, 2020

humphd mentioned this pull request Oct 12, 2020

Check if a feed is already added to the feed set before setting it to invalid #654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #462, Integrate inactive blog url and include logic to filter inactive feeds #639

Fixes #462, Integrate inactive blog url and include logic to filter inactive feeds #639

c3ho commented Feb 2, 2020

humphd left a comment

vercel bot commented Feb 4, 2020 •

edited

Loading

c3ho commented Feb 4, 2020 •

edited

Loading

c3ho commented Feb 4, 2020 •

edited

Loading

c3ho commented Feb 4, 2020

humphd Feb 4, 2020

c3ho Feb 4, 2020

humphd Feb 4, 2020

c3ho Feb 4, 2020

humphd commented Feb 4, 2020

c3ho commented Feb 4, 2020

humphd left a comment

humphd left a comment

humphd left a comment

c3ho commented Feb 6, 2020

c3ho commented Feb 7, 2020

Fixes #462, Integrate inactive blog url and include logic to filter inactive feeds #639

Fixes #462, Integrate inactive blog url and include logic to filter inactive feeds #639

Conversation

c3ho commented Feb 2, 2020

Type of Change

Description

Checklist

humphd left a comment

Choose a reason for hiding this comment

vercel bot commented Feb 4, 2020 • edited Loading

c3ho commented Feb 4, 2020 • edited Loading

c3ho commented Feb 4, 2020 • edited Loading

c3ho commented Feb 4, 2020

humphd Feb 4, 2020

Choose a reason for hiding this comment

c3ho Feb 4, 2020

Choose a reason for hiding this comment

humphd Feb 4, 2020

Choose a reason for hiding this comment

c3ho Feb 4, 2020

Choose a reason for hiding this comment

humphd commented Feb 4, 2020

c3ho commented Feb 4, 2020

humphd left a comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

c3ho commented Feb 6, 2020

c3ho commented Feb 7, 2020

vercel bot commented Feb 4, 2020 •

edited

Loading

c3ho commented Feb 4, 2020 •

edited

Loading

c3ho commented Feb 4, 2020 •

edited

Loading