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
Treat 403s from Flickr as dead links #1201
Changes from all commits
2ecb187
bc898e5
69cbfdc
94b2a65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
"""Per-provider HTTP status mappings for link availability.""" | ||
|
||
from collections import defaultdict | ||
from dataclasses import dataclass | ||
|
||
|
||
@dataclass | ||
class StatusMapping: | ||
unknown: tuple[int] = (429, 403) | ||
live: tuple[int] = (200,) | ||
|
||
|
||
provider_status_mappings = defaultdict( | ||
StatusMapping, | ||
thingiverse=StatusMapping( | ||
# https://github.com/WordPress/openverse/issues/900 | ||
unknown=(429,), | ||
), | ||
flickr=StatusMapping( | ||
# https://github.com/WordPress/openverse/issues/1200 | ||
unknown=(429,), | ||
), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
services: | ||
es: | ||
# Memory limit for ES, as it tends to be a memory hoarder | ||
mem_limit: 4294967296 | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -220,10 +220,6 @@ services: | |
nofile: | ||
soft: 65536 | ||
hard: 65536 | ||
# Memory limit for ES, as it tends to be a memory hoarder | ||
# Set this value to an empty string to remove the limit | ||
# https://docs.docker.com/compose/compose-file/compose-file-v2/#cpu-and-other-resources | ||
mem_limit: ${ES_MEM_LIMIT:-4294967296} # 4 GiB in bytes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this problem only affects your setup (which is likely because it's not been brought up before) the better option would be for you to set the |
||
volumes: | ||
- es-data:/usr/share/elasticsearch/data | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ IS_PROD := env_var_or_default("PROD", env_var_or_default("IS_PROD", "")) | |
# `PROD_ENV` can be "ingestion_server" or "catalog" | ||
PROD_ENV := env_var_or_default("PROD_ENV", "") | ||
IS_CI := env_var_or_default("CI", "") | ||
DC_USER := env_var_or_default("DC_USER", "opener") | ||
ENABLE_DC_OVERRIDES := env_var_or_default("OPENVERSE_ENABLE_DC_OVERRIDES", "true") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not in favour of splitting the Docker Compose config into a regular and an override file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can remove the memory limit entirely, then I'd also prefer not to have an overrides file, but I can't get any mem_limit value to work on my local, even set as high as 32 GB. Docker always kills the container. FWIW, can you share why you dislike an overrides file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just means you have to read in two places to read to mentally piece together a full config. I'm not vetoing it, just expressing displeasure that we had to use this approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, that is sad to hear. It's really frustrating when these differences of OS happens, but related to that and the next quote:
I assume that newer versions have better compatibility between different OS, specially for working with |
||
|
||
# Show all available recipes, also recurses inside nested justfiles | ||
@_default: | ||
|
@@ -115,6 +117,9 @@ DOCKER_FILE := "-f " + ( | |
else { "docker-compose.yml" } | ||
} | ||
else { "docker-compose.yml" } | ||
) + ( | ||
if ENABLE_DC_OVERRIDES == "true" { " -f docker-compose.overrides.yml" } | ||
else { "" } | ||
) | ||
EXEC_DEFAULTS := if IS_CI == "" { "" } else { "-T" } | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is removed in newer docker compose file versions.1
Specifying the value in high-level units is also possible if you still want to keep it.
Footnotes
Compose file reference > Legacy versions > About versions and upgrading ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happy to remove it, but I assume some people rely on this to prevent ES from sucking up their RAM? We're still on compose file version 2.4 for now:
openverse/docker-compose.yml
Line 1 in 2ecb187
When we update the compose format, maybe then we can remove it? I don't want to cause a disruption for others with this PR in the mean time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krysal there was an open issue with higher units not working, so we had to resort to specifying the value as a plain number.
Personally I have no intention of bumping the Docker Compose config up to v3, none of the features we need are absent in v2 and none of the features added to v3 are useful to us.
@sarayourfriend I'd like to get @AetherUnbound's input on this as another Linux user but for macOS folks using Docker Desktop, the memory limit for the system as a whole can be configured from the settings UI.