Fetch all available news from Tü News - #4440
Conversation
|
I would like to include this PR into the release |
jonbulz
left a comment
There was a problem hiding this comment.
You're setting up an infinite loop in the tests. The mock news_manager return value is always 200, thus the exit condition of the loop is never met.
Also, if they ever introduce rate-limiting, we are likely to fall into it. This loop fires rapid requests until if fetches everything for all languages. I just checked and arabic alone went to page 15, meaning 15 requests and ~1500 news items just for this one language. Maybe we should sleep between requests to avoid accidentaly DoS-ing them?
Also, a general remark: Do we really need all news items? The API has a after parameter that would allow us to fetch news of e.g. the latest month or so. Wouldn't that be sufficient as news are, you know, supposed to be news?
@steffenkleinle |
|
As far as I understood it, the plan was to cache/save the news in the CMS, so wouldn't it be possible to fill the database over time to avoid rate limiting etc? If that is the case, I am not sure why we would need to limit it to one month only. But if it helps you, obviously implement a cutoff. I was just mentioning that currently only 5 tünews are included in the news endpoint, which is barely one week and imo too little. |
|
In the discussion in the original issue #4174 we decided against saving news in our database. Can you describe why it is a breaking change if not every and all tünews are available? As far as I understood, the point that we only need the more recent news was made as an argument in favor of the cache option |
Can't say anything to that.
Breaking change was maybe the wrong naming, however, it will be a change to users since currently all tünews are available in the app.
I don't know about that, but on the conference our talks seemed like it would not be a problem to cache all news/load more news from the tünews backend on demand. In the end, I actually don't mind, just thought it would be nice/in line with the current app capabilities and I don't see a downside from an app/user perspective that all news are available. However, if it is a problem on the CMS side, we can just do the last month. |
|
@MizukiTemma not sure if its related, but currently the endpoint is again not returning any tünews:https://cms-test.integreat-app.de/api/v3/testumgebung/de/news/?page=1&count=20&source=tunews |
I ran the management command. It should be working again. |
ce73c81 to
e7c69a6
Compare
|
I'm sorry, but it's just really hard to make good architecture decisions if the requirements come one by one, or are changing. The logic is already a bit bloated because we had to implement an additional single news endpoint and filtering, something which I specifically asked about here and got an answer here. This would have been a lot cheaper if we had saved the news in a Django model from the start.
or
It is possible to do this, but it is not a one line change. Fetching and caching only the latest month or two would hopefully mean that the amount of requests we send stays more or less constant, that's why I proposed it. I'm just highlighting this because it is a good example of how we are inefficient due to a lack of proper communcation. For me, it was again not clear after the first comment if having access to all news is a hard requirement, or just nice to have. Maybe we can learn from this and properly map out the requirements at the start next time, not just the what but also the why, so everybody is on the same page from the beginning. |
|
So while my last comment reads like a bit of a vent, I did some math and with the current number of posts on tunews, it should be possible to always fetch all news if we think that's necessary. I still want to highlight that this does not scale well, though. The way it's implemented in this PR, we do the requests sequentially, so we won't overload their server with it (unless the response status code is != 200, but that's something we can fix). But another issue is that this also steadily increases the processing and allocated memory on our side on every read. The allocated memory per |
|
Thank you for opinions 🙏 We are discussing based on the fectors that are neither officially stated nor we can influence: whether rate limiting will be introduced (if so, what is the max rate), how much news there will be in future in Tü News (whether old news are removed regually or news posts accumulate endlessly), etc. If the compliance to the current behaviour is the only reason to fetch all news posts, I would like to discuss validity of this requirement first. @osmers |
|
I checked whether we have any external requirements to fetch all and apparently we do not. So how about we use the same limit as for internal news, i.e. 28 days? Anything older gets removed/deleted? |
I would like to go for |
I am sorry for that, but I guess that was a misunderstanding then. I only took the question in regards to endpoints with multiple news (e.g. This would have been a lot cheaper if we had saved the news in a Django model from the start. The way the caching currently works is one key per source per language, e.g.
Thanks for explaining. With that information, I'd have said from the start that its not necessary to have all news. I just took the talks on the conference in a very different way since there it was mentioned that is basically no problem to just cache all news and populate it piece by piece.
Yes, totally agree. There was basically no communication and just assumptions on both sides :D |
For sure, that works. In that case, however, we should maybe just always fetch all news the app instead of using pagination. Shouldn't be too many items in that case. I know that this is then another changing requirement, but would it then be possible to just return all items if no I assume that we then also apply the same 28 day logic for amal news? |
The number of days can be set per news source. It doesn't have to be the same for all news sources. We don't know how much news posts there will be in Amal News. They are currently providing only one single post for testing for us. If they regularly clean up ther post lists We probably don't need any freshness restriction and fetch all without thinking about rate limit etc. |
7a8391f to
3c6c8ff
Compare
|
I have updated the description with the deviations decided in the above discussion. @jonbulz |
2ba1a2a to
fa497c9
Compare
f92f6ca to
252dd81
Compare
jonbulz
left a comment
There was a problem hiding this comment.
Here's my re-review. Sorry, it's a lot 😅
Thanks for taking this on, I think we are definitely on the right track!
But what needs fixing is the infinite loop if we receive RequestExceptions. The cache wipe on failed imports also doesn't sit right with me, and the tests are very brittle like this, see my comments. I wouldn't feel comfortable merging until that is addressed.
The rest is a bit more nit-picky. Feel free to reach out in case you want to discuss anything! :)
a321b87 to
6842e6d
Compare
|
I'm investigating the faling tests |
|
@jonbulz |
jonbulz
left a comment
There was a problem hiding this comment.
Thanks again! Almost there 😅
I tried to be more clear about what I mean, I hope that helps. Except for the last 2 comments, everything else is resolved now :)
|
@jonbulz Thank you for explanation 👍 Your sugegstions are applied :) |
jonbulz
left a comment
There was a problem hiding this comment.
Thank you for sticking with me and applying all my suggestions! Looks good now :)
hannaseithe
left a comment
There was a problem hiding this comment.
Thank you, I have some small change requests.
|
@hannaseithe |
hannaseithe
left a comment
There was a problem hiding this comment.
This looks good to me. Thank you Mizuki!
7799574 to
09e7f84
Compare
Co-authored-by: hannaseithe <hannaseithe@users.noreply.github.com>
09e7f84 to
e8b5a21
Compare
Short description
This PR updates
import_news_itemsofTunewsManagerso all the available news posts are fetched.Proposed changes
pageandper_pageparameters to check all news postsSide effects
Faithfulness to issue description and design
There are no intended deviations from the issue and design.During review we decided for some further changes:
TUNEWS_HISTORY_DAYSdays (with defaultTUNEWS_HISTORY_DAYS = FCM_HISTORY_DAYS = 28)sizeparameter is given in the common news endpointnews/How to test
integreat-cms-cli import_externa_newsResolved issues
Fixes: #4439
Updates
Debug code lines from (this commit](ae3bfd6) were moved.
Pull Request Review Guidelines