Improve video controllers caching and retrieving #5709

rhymes · 2020-01-24T17:44:10Z

What type of PR is this? (check all applicable)

Description

This PR has been a little bit of a ride, through it I found out we have some inefficiencies in https://dev.to/videos (and likely other pages) where we fetch the same page (made by 24 items) 3 times (once in the server and twice in the frontend).

This PR though addresses another set of things:

/api/videos fetches only the fields it needs to render the JSON, doesn't do a N + 1 on the user, has flexible pagination and sets the correct edge caching, removing server side caching
/videos fetches only the fields needed, removes the SQL queries that were issued in the partial resulting in N + 1 and sets the correct edge caching

I did not address the issues with multiple fetchings in /videos as it's outside the scope of this PR and it's probably a much bigger refactoring.

The PR is essentially done but it incorporates #5673 so I'm tagging this as WIP until that one is merged and then rebase and force push this one

rhymes · 2020-01-24T17:51:06Z

Writing down some of my discoveries, I'll open a separate issues once I get to the bottom of these:

if you notice insertNext insertVideos and insertArticles (though in a way I'm not understanding) in app/assets/javascripts/initializers/initScrolling.js.erb check if the item has already been inserted:

https://github.com/thepracticaldev/dev.to/blob/3412af977f26a69cb271095808b43b40db229ef2/app/assets/javascripts/initializers/initScrolling.js.erb#L36-L42

https://github.com/thepracticaldev/dev.to/blob/3412af977f26a69cb271095808b43b40db229ef2/app/assets/javascripts/initializers/initScrolling.js.erb#L130-L136

also the fetchers fetch from page 0 and then page 1 which are the same page

mstruve

🔥

app/controllers/api/v0/videos_controller.rb

mstruve · 2020-01-24T21:26:05Z

app/controllers/api/v0/videos_controller.rb

+        num = [per_page, 1000].min
+
+        @video_articles = Article.with_video.
+          includes([:user]).


mstruve · 2020-01-24T21:26:43Z

app/views/videos/index.html.erb

@@ -30,7 +30,7 @@
            <span class="video-timestamp"><%= video_article.video_duration_in_minutes %></span>
          </div>
          <p><strong><%= video_article.title %></strong></p>
-          <p class="video-username"><%= User.find(video_article.user_id).name %></p>


mstruve

🔥

joshpuetz

Very nice!

joshpuetz · 2020-01-27T14:21:21Z

app/controllers/api/v0/videos_controller.rb

-          where("score > ?", -4).
+        page = params[:page]
+        per_page = (params[:per_page] || 24).to_i
+        num = [per_page, 1000].min


This is lovely refactoring!

joshpuetz · 2020-01-27T14:31:58Z

app/models/article.rb

@@ -138,6 +138,8 @@ class Article < ApplicationRecord

  scope :feed, -> { published.select(:id, :published_at, :processed_html, :user_id, :organization_id, :title, :path) }

+  scope :with_video, -> { published.where.not(video: [nil, ""], video_thumbnail_url: [nil, ""]).where("score > ?", -4) }


vaidehijoshi

great work, and great catch on fixing some of the funny stuff that was happening here! ✨

vaidehijoshi · 2020-01-27T15:06:04Z

app/models/article.rb

@@ -138,6 +138,8 @@ class Article < ApplicationRecord

  scope :feed, -> { published.select(:id, :published_at, :processed_html, :user_id, :organization_id, :title, :path) }

+  scope :with_video, -> { published.where.not(video: [nil, ""], video_thumbnail_url: [nil, ""]).where("score > ?", -4) }


vaidehijoshi · 2020-01-27T15:08:10Z

app/controllers/api/v0/videos_controller.rb

@@ -1,21 +1,25 @@
 module Api
  module V0
    class VideosController < ApiController
-      caches_action :index,


Lol, I had never seen caches_action before and had to look up where it was coming from 🙈

TIL about actionpack-action_caching and the fact that we use caches_action in podcast_episodes_controller and tags_controller. I'm assuming we can move away from that and just use set_cache_control_headers in place of it?

@vaidehijoshi this is exactly what i've been doing with these latest PRs :) only a few endpoints left and we can remove that gem :D

Rad!! Thanks for doing this 🙌

rhymes added the blocked blocked by internal or external issues label Jan 24, 2020

rhymes requested review from a team and removed request for a team January 24, 2020 17:44

mstruve reviewed Jan 24, 2020

View reviewed changes

rhymes added 7 commits January 26, 2020 22:10

Add tests and improve pagination and caching

35762b1

Add two more specs

53bb6ef

Add .with_video scope

62c8592

Get rid of N+1

1888f63

Only select needed columns

600b128

Improve efficiency of the video page as well

2527c6e

Add missing set_cache_control_headers to API videos#index

de09a25

rhymes changed the title ~~[WIP] Improve video controllers caching and retrieving~~ Improve video controllers caching and retrieving Jan 26, 2020

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 26, 2020

rhymes requested a review from mstruve January 26, 2020 21:16

rhymes removed the blocked blocked by internal or external issues label Jan 26, 2020

mstruve approved these changes Jan 27, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 27, 2020

mstruve requested review from joshpuetz, maestromac and nickytonline January 27, 2020 14:07

joshpuetz approved these changes Jan 27, 2020

View reviewed changes

vaidehijoshi approved these changes Jan 27, 2020

View reviewed changes

rhymes mentioned this pull request Jan 27, 2020

Improve API podcast episodes controller caching and fetching #5772

Merged

5 tasks

benhalpern merged commit 397ef31 into forem:master Jan 27, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 27, 2020

rhymes deleted the rhymes/improve-videos-controllers branch January 28, 2020 08:29

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve video controllers caching and retrieving #5709

Improve video controllers caching and retrieving #5709

rhymes commented Jan 24, 2020

rhymes commented Jan 24, 2020

mstruve left a comment

mstruve Jan 24, 2020

mstruve Jan 24, 2020

mstruve left a comment

joshpuetz left a comment

joshpuetz Jan 27, 2020

joshpuetz Jan 27, 2020

vaidehijoshi Jan 27, 2020

vaidehijoshi left a comment

vaidehijoshi Jan 27, 2020

vaidehijoshi Jan 27, 2020

rhymes Jan 27, 2020 •

edited

Loading

vaidehijoshi Jan 27, 2020

		@@ -138,6 +138,8 @@ class Article < ApplicationRecord

		scope :feed, -> { published.select(:id, :published_at, :processed_html, :user_id, :organization_id, :title, :path) }

		scope :with_video, -> { published.where.not(video: [nil, ""], video_thumbnail_url: [nil, ""]).where("score > ?", -4) }

Improve video controllers caching and retrieving #5709

Improve video controllers caching and retrieving #5709

Conversation

rhymes commented Jan 24, 2020

What type of PR is this? (check all applicable)

Description

rhymes commented Jan 24, 2020

mstruve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

joshpuetz 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

vaidehijoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhymes Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhymes Jan 27, 2020 •

edited

Loading