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

Update feed only if nextUpdateTime has been reached #2999

Merged
merged 6 commits into from
Dec 30, 2024
Merged

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Dec 22, 2024

Summary

This changes the logic of updating/fetching a feed to check for the nextUpdateTime before the the update is executed.

If that time is not reached yet the feed is not updated. This is mainly a advantage for feeds that are updated rarely and will therefore receive less requests. For the nextcloud instance this is also a benefit because feed updates that are likely not needed are skipped, which saves compute time.

The default minimum for an active feed is "in 1 hour" which means it will be updated in hour again.

Also added a setting to disable this, mostly for testing but I can also imagine that in the beginning or for certain use cases people want to disable this.

Builds on top of #2993

Checklist

@Grotax Grotax marked this pull request as draft December 22, 2024 11:03
@Grotax Grotax force-pushed the updater/newlogic branch 2 times, most recently from d115ee0 to 2d16949 Compare December 23, 2024 13:44
@Grotax Grotax changed the title Updater/newlogic Update feed only if nextUpdateTime has been reached Dec 23, 2024
@Grotax
Copy link
Member Author

Grotax commented Dec 23, 2024

I'm currently struggling a bit with this for multiple reasons.

  1. In my dev environment nextUpdateTime in the DB ends up being Null for some reason for some feeds while in the debug log I see that the nextUpdateTime was logged
  2. Also in the Updater test that I added the field of that feed is null after adding it which i expected but then it stays null after running an update it should no longer be null
  3. If it was working I'm not sure how to test the update behavior since the date would be in one hour, because feed-io sets that as by default as the smallest nextUpdateTime which i think makes sense for real world. But for most of the updater tests that would mean they break.

Anyway can someone tell me why the field can end up being null?

@Grotax

This comment was marked as outdated.

@Grotax
Copy link
Member Author

Grotax commented Dec 29, 2024

Found the issue who would have thought that the Feed-Service does not use the feed object of the fetcher but has it's own and you need to transfer the value :). Which makes me wonder why it worked sometimes. That must have been a different path in the code that causes the value to be passed through ...

@Grotax

This comment was marked as outdated.

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax marked this pull request as ready for review December 29, 2024 15:09
@Grotax Grotax requested a review from SMillerDev December 29, 2024 15:10
Copy link
Contributor

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

All looks good to me, thanks for all the time you put into this!

@Grotax Grotax merged commit 812a219 into master Dec 30, 2024
24 of 25 checks passed
@Grotax Grotax deleted the updater/newlogic branch December 30, 2024 10:46
Grotax added a commit that referenced this pull request Dec 30, 2024
Changed
- API add new field to Feed that indicates when the next update will be done "nextUpdateTime" (#2993)
- Change logic to update feed only if the nextUpdateTime has been reached (#2999)
- Add setting to disable the usage of nextUpdateTime (#2999)

Fixed
- `TypeError: this.$refs.actions.$refs.menuButton is undefined` when tabbing through feeds and folders (#3004)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Dec 30, 2024
Grotax added a commit that referenced this pull request Dec 30, 2024
Changed
- API add new field to Feed that indicates when the next update will be done "nextUpdateTime" (#2993)
- Change logic to update feed only if the nextUpdateTime has been reached (#2999)
- Add setting to disable the usage of nextUpdateTime (#2999)

Fixed
- `TypeError: this.$refs.actions.$refs.menuButton is undefined` when tabbing through feeds and folders (#3004)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants