-
Notifications
You must be signed in to change notification settings - Fork 31
feat: Add Redis caching for navigation pagination #484
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
Conversation
107d343
to
88cee0d
Compare
88cee0d
to
d7e2f95
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.
Great stuff here. Just a few thoughts, requests.
"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.
Let's move this to extras so people can do this pip install stac-fastapi-core[redis]
if they need redis. Some may not want to install the library if they are not using 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.
I have moved redis
into extras, thank you!
stac_fastapi/elasticsearch/setup.py
Outdated
"elasticsearch[async]~=8.18.0", | ||
"uvicorn~=0.23.0", | ||
"starlette>=0.35.0,<0.36.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.
I think we can remove redis from elasticsearch, opensearch and sfeos_helpers
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.
stac_fastapi/opensearch/setup.py
Outdated
"opensearch-py[async]~=2.8.0", | ||
"uvicorn~=0.23.0", | ||
"starlette>=0.35.0,<0.36.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.
I think we can remove redis from elasticsearch, opensearch and sfeos_helpers
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.
stac_fastapi/sfeos_helpers/setup.py
Outdated
|
||
install_requires = [ | ||
"stac-fastapi.core==6.5.1", | ||
"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.
remove here 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.
done.
|
||
> [!NOTE] | ||
> Use either the Sentinel configuration (`REDIS_SENTINEL_HOSTS`, `REDIS_SENTINEL_PORTS`, `REDIS_SENTINEL_MASTER_NAME`) OR the Redis configuration (`REDIS_HOST`, `REDIS_PORT`), but not both. | ||
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 add a REDIS_ENABLED global env var to turn on/off redis. The try/except blocks can add latency to the application if someone doesn't want to use Redis and the api keeps trying to connect.
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 introduced REDIS_ENABLED
env var that is used to indicate if redis has been configured and enabled.
|
||
current_url = str(request.url) | ||
redis = None | ||
try: |
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.
before the try
we should check to see if REDIS_ENABLED is 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.
Fixed, thank you!
HTTPException: If there is an error with the cql2_json filter. | ||
""" | ||
base_url = str(request.base_url) | ||
try: |
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.
here too, check REDIS_ENABLED
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.
Fixed, thank you!
""" | ||
base_url = str(request.base_url) | ||
try: | ||
redis = await 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.
connect_redis here too I think?
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.
Fixed, thank you!
stac_fastapi/tests/api/test_api.py
Outdated
|
||
result_missing = await get_prev_link(mock_redis, "dummy_token_2") | ||
assert result_missing is None | ||
mock_redis.get.assert_awaited_once_with("nav:self:dummy_token_2") |
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.
@YuriZmytrakov Should we have integration testing with a live redis server? We would need to set up redis in gh actions too. I think it's a good idea.
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.
dockerfiles/Dockerfile.dev.es
Outdated
RUN pip install --no-cache-dir -e ./stac_fastapi/core | ||
RUN pip install --no-cache-dir -e ./stac_fastapi/sfeos_helpers | ||
RUN pip install --no-cache-dir -e ./stac_fastapi/elasticsearch[dev,server] | ||
RUN pip install --no-cache-dir redis types-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.
do we need this? we should create a new compose.yml file and add redis to it for local testing I think - compose-redis.yml
or something like that.
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.
Here is a compose config for a redis cache
redis:
image: redis:alpine
ports:
- "6379:6379"
volumes:
- redis_data:/data
volumes:
redis_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.
This was fixed, thank you @jonhealy1 . Thank you for the suggested code @jamesfisher-geo .
links = await PagingLinks(request=request, next=next_token).get_links() | ||
|
||
collection_links = [] | ||
if ( |
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.
Is this needed? I believe at least the parent
link is automatically created by the API with https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/blob/main/stac_fastapi/core/stac_fastapi/core/models/links.py#L125
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.
Yes, unfortunately this was reported as a bug, the parent
and collection
relations are missing. For this reason, I’m resolving it here. This was the original issue for this ticket, which eventually evolved into the Redis cache for navigation project.
await save_self_link(redis, next_token, self_link) | ||
|
||
prev_link = await get_prev_link(redis, token_param) | ||
if 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.
How do you feel about handling the previous
paging link additions inside the PagingLinks
class? This would be more maintainable from a code perspective.
You could also pass the previous
link into get_links
with the extra_links
parameter?
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 point, I tried this approach as well. However, I’d need to pass the Redis
connection into the PagingLinks
class. Also, the token for the previous link is derived from the current
token, which isn’t available in PagingLinks
it’s only accessible in the current link that I retrieve in the endpoint where caching is enabled. Hope this makes sense.
dockerfiles/Dockerfile.dev.es
Outdated
RUN pip install --no-cache-dir -e ./stac_fastapi/core | ||
RUN pip install --no-cache-dir -e ./stac_fastapi/sfeos_helpers | ||
RUN pip install --no-cache-dir -e ./stac_fastapi/elasticsearch[dev,server] | ||
RUN pip install --no-cache-dir redis types-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.
Here is a compose config for a redis cache
redis:
image: redis:alpine
ports:
- "6379:6379"
volumes:
- redis_data:/data
volumes:
redis_data:
42d6c98
to
c056704
Compare
**Related Issue(s):** - stac-utils#461 **Description:** - GET `/collections` collection search fields extension ex. `/collections?fields=id,title`. [stac-utils#465](stac-utils#465) - Improved error messages for sorting on unsortable fields in collection search, including guidance on how to make fields sortable. [stac-utils#465](stac-utils#465) - Added field alias for `temporal` to enable easier sorting by temporal extent, alongside `extent.temporal.interval`. [stac-utils#465](stac-utils#465) **PR Checklist:** - [x] Code is formatted and linted (run `pre-commit run --all-files`) - [x] Tests pass (run `make test`) - [x] Documentation has been updated to reflect changes, if applicable - [x] Changes are added to the changelog
**Related Issue(s):** - stac-utils#468 **Description:** **PR Checklist:** - [x] Code is formatted and linted (run `pre-commit run --all-files`) - [x] Tests pass (run `make test`) - [x] Documentation has been updated to reflect changes, if applicable - [x] Changes are added to the changelog
**Related Issue(s):** - stac-utils#460 **Description:** ex. `/collections?q=Sentinel-2a` **PR Checklist:** - [x] Code is formatted and linted (run `pre-commit run --all-files`) - [x] Tests pass (run `make test`) - [x] Documentation has been updated to reflect changes, if applicable - [x] Changes are added to the changelog
**Related Issue(s):** - None **Description:** - Release v6.4.0 **PR Checklist:** - [x] Code is formatted and linted (run `pre-commit run --all-files`) - [x] Tests pass (run `make test`) - [x] Documentation has been updated to reflect changes, if applicable - [x] Changes are added to the changelog
a9c440e
to
64bf85b
Compare
@jonhealy1 @jamesfisher-geo apologies, I have to close this PR and cherry-pick the changes into a new one. This PR has been sitting here for a while, and any change now triggers too many conflicts to resolve. |
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
)