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

Adjust day to fetch new geo location databases when done monthly #21468

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

PowerKiKi
Copy link
Contributor

Description:

When geolocation database updates are configured to be done monthly, they were scheduled on the 3rd day of the week, so Wednesday. For months that start on a Wednesday, that means that we tried to fetch a file that might not exist yet.

We now schedule on the 3rd day of the month, as I believe was the original intent of dc98a97 that introduced the behavior a long time ago. So this gives 3 days to db-ip.com to update their files in all cases.

Fixes #18427

Review

When geolocation database updates are configured to be done monthly,
they were scheduled on the 3rd day of _the week_, so Wednesday. For
months that start on a Wednesday, that means that we tried to fetch a
file that might not exist yet.

We now schedule on the 3rd day of _the month_, as I believe was the
original intent of dc98a97 that introduced the behavior a long time
ago. So this gives 3 days to db-ip.com to update their files in all
cases.

Fixes matomo-org#18427
@PowerKiKi
Copy link
Contributor Author

I think the CI failures are unrelated to this PR, but could somebody confirm that, please ?

@michalkleiner
Copy link
Contributor

Thank you for the contribution @PowerKiKi. My instinct says this would help with the issue but doesn't really address the possibility the files are not yet available, regardless of what day of the month or week the check is performed. I'd say we should address that rather than just pushing the check a few days back.

@PowerKiKi
Copy link
Contributor Author

That makes senses to also better handle the failed fetch. But I think they are complementary.

The present situation where the delay is between 0 to 6 days, "randomly", depending on the current month, does not make any sense to me. So I think it should be fixed anyway.

Then, in addition to that, having a mechanism that gracefully handle failure by, maybe, trying again the next day, until success, could be a further improvement. But I don't think I'd have time to work on that myself.

@tassoman
Copy link
Contributor

tassoman commented Nov 2, 2023

@PowerKiKi solution is an efficient edit to fix an actual software malfunction. The service providing GEOIP updates, may needs from 3 to 6 hours to get things done. Also it rarely happens.
So I wouldn't address more effort than this, because, having it logged as an ERROR it's already enough to manage manually.
Having a feature for 404 management of GEOIP data it's out the scope of scheduled jobs.

@sgiehl
Copy link
Member

sgiehl commented Nov 2, 2023

I'm going to merge this one. Using setDayOfWeek before was imho more likely a mistake and setDay is more "correct". We anyway should aim to implement a proper handling of non existing databases at a later step.

@sgiehl sgiehl merged commit 52f3416 into matomo-org:5.x-dev Nov 2, 2023
22 of 25 checks passed
@sgiehl sgiehl changed the title Don't fetch geolocation database that may not exist yet Adjust day to fetch new geo location databases when done monthly Nov 2, 2023
@sgiehl sgiehl added this to the 5.0.0 milestone Nov 2, 2023
@sgiehl sgiehl added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Nov 2, 2023
@PowerKiKi PowerKiKi deleted the issue-18427 branch November 2, 2023 13:42
caddoo pushed a commit that referenced this pull request Nov 5, 2023
When geolocation database updates are configured to be done monthly,
they were scheduled on the 3rd day of _the week_, so Wednesday. For
months that start on a Wednesday, that means that we tried to fetch a
file that might not exist yet.

We now schedule on the 3rd day of _the month_, as I believe was the
original intent of dc98a97 that introduced the behavior a long time
ago. So this gives 3 days to db-ip.com to update their files in all
cases.

Fixes #18427
caddoo pushed a commit that referenced this pull request Nov 5, 2023
When geolocation database updates are configured to be done monthly,
they were scheduled on the 3rd day of _the week_, so Wednesday. For
months that start on a Wednesday, that means that we tried to fetch a
file that might not exist yet.

We now schedule on the 3rd day of _the month_, as I believe was the
original intent of dc98a97 that introduced the behavior a long time
ago. So this gives 3 days to db-ip.com to update their files in all
cases.

Fixes #18427
caddoo pushed a commit that referenced this pull request Nov 5, 2023
When geolocation database updates are configured to be done monthly,
they were scheduled on the 3rd day of _the week_, so Wednesday. For
months that start on a Wednesday, that means that we tried to fetch a
file that might not exist yet.

We now schedule on the 3rd day of _the month_, as I believe was the
original intent of dc98a97 that introduced the behavior a long time
ago. So this gives 3 days to db-ip.com to update their files in all
cases.

Fixes #18427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GeoIP2 updater might try to download new file before it exists
4 participants