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

feat: Add caching mechanism to optimize weather data fetching from Op… #197

Closed

Conversation

VinVorteX
Copy link

@VinVorteX VinVorteX commented Oct 5, 2024

Issue: #196

Motivation and Context

This PR introduces caching to the weather backend using the OpenMeteo API. It improves performance by storing weather data for each location, reducing the number of API calls and ensuring data freshness for a specific period. It addresses the need to limit repeated API requests within a short time frame, enhancing the application's efficiency.

Key Benefits:

Reduced API request load.
Faster response times for repeated requests.
Improved resource management and performance.
Description

Added caching functionality:

Introduced a cache map to store weather data by location, along with timestamps.
Data remains valid for a specified cacheDuration (set to 1 hour).
Before making API calls, the cache is checked for existing, valid data.
Enhanced the Fetch function:

It now checks the cache before fetching fresh data from the OpenMeteo API.
If the cache contains valid data (based on timestamp), it returns cached data.
Otherwise, it fetches new data, updates the cache, and returns the updated result.
Synchronization using mutex locks:

Added a sync.Mutex to ensure thread-safe access to the cache map, preventing race conditions in concurrent requests.
Steps for Testing

Setup:

Pull the latest changes from this branch.
Ensure the Go environment is properly set up with required dependencies (e.g., github.com/schachmat/wego/iface).
Testing the caching functionality:

Run the backend and make a request to the OpenMeteo API for a specific location.
Check the logs to confirm that a fresh API request is made.
Wait less than 1 hour and make the same request for the same location again. The cache should be used, reducing the API call.
Wait for more than 1 hour, and ensure that a new API request is made after cache expiration.
Check concurrency:

Simulate multiple concurrent requests for the same location to verify that caching works without race conditions or deadlocks.
Screenshots (if applicable)

N/A for this PR (backend changes are not visual).

@@ -161,11 +171,22 @@ func parseCurCond(current curCond) (ret iface.Cond) {

}

var mu sync.Mutex
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need a mutex? None of the code uses any goroutines or is parallelized from what I know?

Copy link
Author

@VinVorteX VinVorteX Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! The mutex is there to keep the cache safe if we get multiple requests at the same time. Even though we’re not using goroutines now, it’s a good precaution for the future. I can look into it again if needed!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest remove it given its not useful right now. It's "dead code"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern about the mutex since we aren't using goroutines in the current implementation. I can remove it to simplify the code. Thanks for the feedback!

@@ -110,6 +118,8 @@ var (
func (opmeteo *openmeteoConfig) Setup() {
flag.StringVar(&opmeteo.apiKey, "openmeteo-api-key", "", "openmeteo backend: the api `KEY` to use if commercial usage")
flag.BoolVar(&opmeteo.debug, "openmeteo-debug", false, "openmeteo backend: print raw requests and responses")
opmeteo.cache = make(map[string]cachedData) // initializing a cache
opmeteo.cacheDuration = 1 * time.Hour // setting the cache duation to 1 hour
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the cache duration be configurable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Making the cache duration configurable would give users more flexibility. I can add that feature to allow them to set it according to their needs.

@VinVorteX VinVorteX closed this Oct 6, 2024
@VinVorteX VinVorteX force-pushed the feature/add-caching-mechanism branch from 5b6d45c to 6578d5e Compare October 6, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants