Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Home from db after limit #5679

Closed

Conversation

ClearlyClaire
Copy link
Contributor

This change allows to scroll back past the 400 toots limit of the Redis cache for the Home Timeline.
Indeed, some users like to read all the toots from their timeline, but may be away long enough for it to exceed the 400 toots limit.

This PR is tagged as work in progress as it may be greatly inefficient---although that only affects people actually scrolling past those 400 toots or waiting for the cache to be repopulated.

I guess it would be ideal to move the various filters to the SQL query in Status#as_home_timeline.

@ClearlyClaire ClearlyClaire added the work in progress Not to be merged, currently being worked on label Nov 13, 2017
@ClearlyClaire ClearlyClaire force-pushed the home-from-db-after-limit branch 4 times, most recently from f841aab to 1e12659 Compare November 13, 2017 21:17
@ClearlyClaire
Copy link
Contributor Author

I currently have no idea what's going on with the failing tests, to be honest.

Status.as_home_timeline(@account)
.paginate_by_max_id(limit, max_id, since_id)
.reject { |status| FeedManager.instance.filter?(:home, status, @account.id) }
for attempt in 0..20 do
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for attempt in 0..20 do -> 21.times do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks! This commit is not needed, though, I just tried to avoid the test failures, but I still don't understand it. I will drop that commit.

@ClearlyClaire ClearlyClaire force-pushed the home-from-db-after-limit branch 3 times, most recently from 84d37bf to d8610db Compare November 15, 2017 17:31
@ClearlyClaire
Copy link
Contributor Author

Fixed the test (since it now passes). This is still sub-optimal as it may return less statuses than requested while performing potentially useless SQL queries, but it works.

@ClearlyClaire ClearlyClaire removed the work in progress Not to be merged, currently being worked on label Nov 16, 2017
@ClearlyClaire
Copy link
Contributor Author

Removed the “work in progress” label as I'm probably not going to work on the SQL side of things.
It works reasonably well for my single-user instance, not sure how bad it would be performance-wise in practice.

@akihikodaki
Copy link
Contributor

Duplicate of #4016?

@ClearlyClaire
Copy link
Contributor Author

@akihikodaki yes, indeed, gonna close this one, but you want to fix from_database not returning anything if the first database results are filtered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants