-
Notifications
You must be signed in to change notification settings - Fork 31
feat: Add Redis caching for navigation pagination #488
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
base: main
Are you sure you want to change the base?
Conversation
5c8d201
to
8991cfc
Compare
b16cc88
to
8991cfc
Compare
|
||
if redis_enable: | ||
try: | ||
redis = await connect_redis() |
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.
how does this work with connect_redis_sentinel?
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.
there is connect_redis and connect_redis_sentinel. I think maybe there is logic missing which should determine which one to use? Maybe you can explain how it works?
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.
we should also log the redis connection whether its successful or not?
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.
-
If a user wants to use standalone Redis (the most common case), they should use
connect_redis()
. If they want to use Redis Sentinel (used by Cloudferro),connect_redis_sentinel()
should be used. The connection is configured differently in each case, which is why we have two separate functions. The user needs to configure either theREDIS
orREDIS_SENTINEL
environment variables. -
The selection of which function to use should be done in redis_utils.py:
- For standalone Redis: redis_settings: BaseSettings = RedisSettings()
- For Redis Sentinel: redis_settings: BaseSettings = RedisSentinelSettings()
-
Logging has been added for users.
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.
Ok thanks for explaining - here you only use connect_redis but should you use connect_redis_sentinel as well?
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.
If the user configures Redis Sentinel
, the connect_redis_sentinel
function should be used instead of connect_redis
. I could try to combine these two functions into one, which would choose the appropriate approach depending on whether the user wants standalone or Sentinel depending on config selected. This way, no one would need to manually replace function, only connect_redis
would be called. Both approaches are fine with me. Just to note, Cloudferro will probably be the only users of Sentinel, which is why I added this function.
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.
Good idea, maybe even a helper function that calls the right function. If there are env options for sentinel in the codebase then we have to make it useable. Never say never.
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.
@jonhealy1 this has been implemented. Moving forward, depending on the configuration specified via environment variables, it will be handled by connect_redis
. The developer is not expected to select the function.
"href": prev_link, | ||
}, | ||
) | ||
|
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.
can this code - the redis_enabled block - be put into a function? It is used with all_collections too
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.
The code block was replaced here with func as well. Thank you!
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.
Let's change the function name for handle_pagination_links - also, mentioned below - and let's check the REDIS IS ENABLED here before we go to the function.
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.
Done, thank you!
"href": prev_link, | ||
}, | ||
) | ||
|
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 code into a function
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.
@jonhealy1 I have moved this code into handle_pagination_links
func that can be reused where needed.
8991cfc
to
e3e84cd
Compare
- Configure Redis image for tests of caching navigation - Update Make file Redis with test targets for ES and OS - Integrate Redis/Redis Sentinel client to cache navigation - Add Redis funcs Sentinel for navigation caching
Add tests for Redis pagination caching in search and collections endpoints, plus utility function tests.
f8a4ad9
to
ff38a98
Compare
f8ed74c
to
17d59cf
Compare
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.
Looking good. Here are some recommendations to simplify the setup
Dockerfile
Outdated
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 don't think this file is needed. The Dockerfile in ./dockerfiles/Dockerfile.deploy.es works
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.
Removed, thank you!
compose-redis.yml
Outdated
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.
You can greatly simplify the testing setup here by including redis in the compose.yml file.
I'll pose the full contents of the compose file that I have tested locally. Note that using an append only cache will cause issues with handling of item delete requests.
Including redis in the central docker-compose will also help future development -- we will be able to spin up all resources with a single docker compose up app-[opensearch | elasticsearch]
command.
services:
app-elasticsearch:
container_name: stac-fastapi-es
image: stac-utils/stac-fastapi-es
restart: always
build:
context: .
dockerfile: dockerfiles/Dockerfile.dev.es
environment:
- STAC_FASTAPI_TITLE=stac-fastapi-elasticsearch
- STAC_FASTAPI_DESCRIPTION=A STAC FastAPI with an Elasticsearch backend
- STAC_FASTAPI_VERSION=6.0.0
- STAC_FASTAPI_LANDING_PAGE_ID=stac-fastapi-elasticsearch
- APP_HOST=0.0.0.0
- APP_PORT=8080
- RELOAD=true
- ENVIRONMENT=local
- WEB_CONCURRENCY=10
- ES_HOST=elasticsearch
- ES_PORT=9200
- ES_USE_SSL=false
- ES_VERIFY_CERTS=false
- BACKEND=elasticsearch
- DATABASE_REFRESH=true
- ENABLE_COLLECTIONS_SEARCH_ROUTE=true
- REDIS_ENABLE=true
- REDIS_HOST=redis
- REDIS_PORT=6379
ports:
- "8080:8080"
volumes:
- ./stac_fastapi:/app/stac_fastapi
- ./scripts:/app/scripts
- ./esdata:/usr/share/elasticsearch/data
depends_on:
- elasticsearch
- redis
command:
bash -c "./scripts/wait-for-it-es.sh es-container:9200 && python -m stac_fastapi.elasticsearch.app"
app-opensearch:
container_name: stac-fastapi-os
image: stac-utils/stac-fastapi-os
restart: always
build:
context: .
dockerfile: dockerfiles/Dockerfile.dev.os
environment:
- STAC_FASTAPI_TITLE=stac-fastapi-opensearch
- STAC_FASTAPI_DESCRIPTION=A STAC FastAPI with an Opensearch backend
- STAC_FASTAPI_VERSION=6.0.0
- STAC_FASTAPI_LANDING_PAGE_ID=stac-fastapi-opensearch
- APP_HOST=0.0.0.0
- APP_PORT=8082
- RELOAD=true
- ENVIRONMENT=local
- WEB_CONCURRENCY=10
- ES_HOST=opensearch
- ES_PORT=9202
- ES_USE_SSL=false
- ES_VERIFY_CERTS=false
- BACKEND=opensearch
- STAC_FASTAPI_RATE_LIMIT=200/minute
- ENABLE_COLLECTIONS_SEARCH_ROUTE=true
- REDIS_ENABLE=true
- REDIS_HOST=redis
- REDIS_PORT=6379
ports:
- "8082:8082"
volumes:
- ./stac_fastapi:/app/stac_fastapi
- ./scripts:/app/scripts
- ./osdata:/usr/share/opensearch/data
depends_on:
- opensearch
- redis
command:
bash -c "./scripts/wait-for-it-es.sh os-container:9202 && python -m stac_fastapi.opensearch.app"
elasticsearch:
container_name: es-container
image: docker.elastic.co/elasticsearch/elasticsearch:${ELASTICSEARCH_VERSION:-8.11.0}
hostname: elasticsearch
environment:
ES_JAVA_OPTS: -Xms512m -Xmx1g
action.destructive_requires_name: false
volumes:
- ./elasticsearch/config/elasticsearch.yml:/usr/share/elasticsearch/config/elasticsearch.yml
- ./elasticsearch/snapshots:/usr/share/elasticsearch/snapshots
ports:
- "9200:9200"
opensearch:
container_name: os-container
image: opensearchproject/opensearch:${OPENSEARCH_VERSION:-2.11.1}
hostname: opensearch
environment:
- discovery.type=single-node
- plugins.security.disabled=true
- OPENSEARCH_JAVA_OPTS=-Xms512m -Xmx512m
- action.destructive_requires_name=false
volumes:
- ./opensearch/config/opensearch.yml:/usr/share/opensearch/config/opensearch.yml
- ./opensearch/snapshots:/usr/share/opensearch/snapshots
ports:
- "9202:9202"
redis:
image: redis:7-alpine
hostname: redis
ports:
- "6379:6379"
volumes:
- redis_test_data:/data
command: redis-server
volumes:
redis_test_data:
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.
Thank you, @jamesfisher-geo , for the recommendation. I added a separate compose-docker.yml
file as suggested in the previous review. Having a separate compose.yml
was helpful when I worked on caching navigation in Redis, as it allowed me to easily run only the Redis tests with a single command. This setup would be beneficial if Redis for navigation expands in the future. For now, I think it’s sufficient to move compose-redis.yml
into compose.yml
for handling only the prev/next
links, but I do see good reasoning for separating it into its own .yml
file.
Makefile
Outdated
run docs | ||
run docs | ||
|
||
.PHONY: test-redis-es |
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.
With the change proposed in compose-redis.yml
, these can be removed.
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.
Removed, ty!
mypy.ini
Outdated
@@ -0,0 +1,3 @@ | |||
[mypy] | |||
[mypy-redis.*] | |||
ignore_missing_imports = True |
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 don't think we want this as it could lead to some bad typing getting pushed into prod. You can add the redis types to pre-commit by adding "redis-types"
to the additional dependencies in the pre-commit config here
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.
Removed.
stac_fastapi/core/setup.py
Outdated
"pygeofilter~=0.3.1", | ||
"jsonschema~=4.0.0", | ||
"slowapi~=0.1.9", | ||
"redis==6.4.0", |
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.
If redis caching is an optional feature, should it also be an optional dependency?
I think this can be added here by moving redis==6.4.0
to
extras_require = {
"redis": ["redis==6.4.0"],
}
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 do get warnings in the log when redis_enable is false that the redis dependency is missing in redis_utils.py when importing. Though it works, it might be a good idea to conditionally import Redis only if it’s installed. I have removed it from install_requires, moving forward, redis will be included in extra_reqs only.
stac_fastapi/elasticsearch/setup.py
Outdated
"pre-commit~=3.0.0", | ||
"ciso8601~=2.3.0", | ||
"httpx>=0.24.0,<0.28.0", | ||
"redis==6.4.0", |
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.
Same comment about an optional dependency
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.
Redis is not needed here in this setup because it is being used in core and it should be installed there as an optional dependency. Same with both ES and OS
stac_fastapi/opensearch/setup.py
Outdated
"pre-commit~=3.0.0", | ||
"ciso8601~=2.3.0", | ||
"httpx>=0.24.0,<0.28.0", | ||
"redis==6.4.0", |
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.
Same comment about an optional dependency
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.
Redis is not needed here in this setup because it is being used in core and it should be installed there as an optional dependency. Same with both ES and OS
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.
@jonhealy1 removing "redis==6.4.0"
from extra_inst
causes this error: ERROR - ModuleNotFoundError: No module named 'redis' in CICD.
Looks like we do need this dependency to be installed?
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.
it should be ok if redis is placed in core in extras, and then we install stac-fastapi-core[redis] in extras for the ES/ OS setup. then in cicd we install stac-fastapi-elasticsearch[redis]
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.
in core:
extras_require={
"redis": ["redis~=6.4.0"],
},
then in es/os:
extras_require={
"redis": ["stac-fastapi-core[redis]==6.5.1"]
}
the original redis should be ~= instead == maybe?
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 see, thank you for elaborating. For local development, developers will be expected to install redis
manually into environment, CICD will install redis via extra_reqs
. Seems to be working this way right now.
"method": "GET", | ||
"href": prev_link, | ||
}, | ||
) |
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.
let's change the name of this function to something like redis_pagination_links.
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 have re named the function to redis_pagination_links
, though I preferred the previous name as it was more descriptive. Or evne handle_pagination_links_redis
, handle_pagination_links_redis_cache
could work, though they might be a too long.
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.
Handle_prev_pagination_links?
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 would not restrict the name to prev
as potentially, the other missing navigation rels might be handled by this func, and it will need to be renamed again. I think redis_pagination_links
is a good option.
"href": prev_link, | ||
}, | ||
) | ||
|
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.
Let's change the function name for handle_pagination_links - also, mentioned below - and let's check the REDIS IS ENABLED here before we go to the function.
88776f9
to
e8e3405
Compare
stac_fastapi/opensearch/setup.py
Outdated
"pre-commit~=3.0.0", | ||
"ciso8601~=2.3.0", | ||
"httpx>=0.24.0,<0.28.0", | ||
"redis==6.4.0", |
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.
Redis is not needed here in this setup because it is being used in core and it should be installed there as an optional dependency. Same with both ES and OS
stac_fastapi/elasticsearch/setup.py
Outdated
"pre-commit~=3.0.0", | ||
"ciso8601~=2.3.0", | ||
"httpx>=0.24.0,<0.28.0", | ||
"redis==6.4.0", |
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.
Redis is not needed here in this setup because it is being used in core and it should be installed there as an optional dependency. Same with both ES and OS
8f58ce8
to
6bc9ee4
Compare
6bc9ee4
to
9cf57b6
Compare
"stac-fastapi-core[redis]==6.5.1", | ||
], | ||
"docs": ["mkdocs~=1.4.0", "mkdocs-material~=9.0.0", "pdocs~=1.2.0"], | ||
"server": ["uvicorn[standard]~=0.23.0"], |
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.
Looks good. Here don't change anything. just add in "redis": ["stac-fastapi-core[redis]==6.5.1"] to the extra_reqs. Then someone can go pip install stac-fastapi-elasticsearch[redis]
---- same thing in opensearch setup
Related Issue(s):
Description:
Add Redis caching support for navigation pagination to enable proper
prev
/next
links in STAC API responses.PR Checklist:
pre-commit run --all-files
)make test
)