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

Remove transition code in search controller #706

Closed
1 task
dhruvkb opened this issue May 16, 2022 · 7 comments · Fixed by #5050
Closed
1 task

Remove transition code in search controller #706

dhruvkb opened this issue May 16, 2022 · 7 comments · Fixed by #5050
Assignees
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: api Related to the Django API

Comments

@dhruvkb
Copy link
Member

dhruvkb commented May 16, 2022

Problem

The code described in this snippet was transitionary and has served its utility. It should be removed.

try:
sources = cache.get(key=source_cache_name)
except ValueError:
cache_fetch_failed = True
sources = None
logger.warning("Source cache fetch failed due to corruption")
except ConnectionError:
cache_fetch_failed = True
sources = None
logger.warning("Redis connect failed, cannot get cached sources.")
if isinstance(sources, list) or cache_fetch_failed:
sources = None
try:
# Invalidate old provider format.
cache.delete(key=source_cache_name)
except ConnectionError:
logger.warning("Redis connect failed, cannot invalidate cached sources.")

Description

This cache invalidation snippet has definitely run in production and the caches have surely been migrated to the new format. This should no longer be a part of the codebase.

Alternatives

Even in the highly unlikely event that the caches have not been cleared and updated, a better solution would be to use try...except to handle invalid values rather than using a very specific transition state.

Additional context

The issue originally pointed to this code snippet from the now-deprecated and archived WordPress/openverse-api repo
https://github.com/WordPress/openverse-api/blob/6cc0551c639081f895d974b30cc465713d0ce360/api/catalog/api/controllers/search_controller.py#L379-L387

Implementation

  • 🙋 I would be interested in implementing this feature.
@dhruvkb dhruvkb added 🟩 priority: low Low priority and doesn't need to be rushed ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository labels May 16, 2022
@sarayourfriend
Copy link
Collaborator

Can we check the production cache to confirm this is safe?

@dhruvkb
Copy link
Member Author

dhruvkb commented May 19, 2022

@sarayourfriend given how long this has existed in the code, it's safe to assume this code has executed at least once, but yeah we can check the cache once and verify.

@obulat obulat transferred this issue from WordPress/openverse-api Feb 22, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Feb 23, 2023
@obulat obulat added 🧱 stack: api Related to the Django API and removed 🧱 stack: backend labels Mar 20, 2023
dhruvkb pushed a commit that referenced this issue Apr 14, 2023
* Always use Jamendo's "streaming" audio

* Simplify functions, fix documentation
@sarayourfriend
Copy link
Collaborator

@dhruvkb can you update this issue with a new link for the monorepo code? At a glance, I think I understand what would need to be removed, but the issue is old enough and the link is to the archive repo, so my confidence that I can understand it is low (which doesn't bode well for many other people outside the team 😅).

@dhruvkb
Copy link
Member Author

dhruvkb commented Jun 1, 2024

@sarayourfriend, done.

@obulat obulat added good first issue New-contributor friendly help wanted Open to participation from the community labels Jun 4, 2024
@dryruffian
Copy link
Contributor

Hey @dhruvkb, I'd like to work on this issue. Can you please assign it to me?

@dhruvkb
Copy link
Member Author

dhruvkb commented Oct 13, 2024

Sure, please go ahead!

@openverse-bot openverse-bot moved this from 📋 Backlog to 📅 To Do in Openverse Backlog Oct 13, 2024
@dryruffian
Copy link
Contributor

Thanks, @dhruvkb. I'm on it!

@openverse-bot openverse-bot moved this from 📅 To Do to 🏗 In Progress in Openverse Backlog Oct 17, 2024
@obulat obulat closed this as completed in fb45bbe Oct 18, 2024
@openverse-bot openverse-bot moved this from 🏗 In Progress to ✅ Done in Openverse Backlog Oct 18, 2024
Danil49 pushed a commit to Danil49/openverse that referenced this issue Oct 29, 2024
…ordPress#5050)

* Remove obsolete cache invalidation code from get_sources function(fixes WordPress#706)

* Revert unrelated changes

Signed-off-by: Olga Bulat <[email protected]>

---------

Signed-off-by: Olga Bulat <[email protected]>
Co-authored-by: Olga Bulat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: api Related to the Django API
Projects
Archived in project
4 participants