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

Issue #937 - Use redis as a shared throwaway cache #1120

Merged
merged 1 commit into from
Feb 13, 2019

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented Feb 13, 2019

Closes #937

@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #1120 into master will decrease coverage by 0.28%.
The diff coverage is 8.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
- Coverage   37.32%   37.04%   -0.29%     
==========================================
  Files          53       53              
  Lines        7729     7734       +5     
==========================================
- Hits         2885     2865      -20     
- Misses       4414     4440      +26     
+ Partials      430      429       -1
Impacted Files Coverage Δ
reposerver/repository/repository.go 29.89% <0%> (+2.96%) ⬆️
server/application/application.pb.gw.go 0% <0%> (ø) ⬆️
util/cache/redis.go 0% <0%> (ø) ⬆️
util/cache/cache.go 0% <0%> (ø)
util/oidc/oidc.go 2.83% <0%> (+0.07%) ⬆️
controller/appcontroller.go 28.74% <3.44%> (+0.81%) ⬆️
server/application/application.go 27.25% <54.54%> (+0.09%) ⬆️
server/server.go 45.67% <75%> (ø) ⬆️
util/argo/argo.go 24.56% <0%> (-2.04%) ⬇️
util/git/client.go 58.53% <0%> (-1.92%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4e4f3...f99b163. Read the comment docs.

}
client := redis.NewClient(&redis.Options{
Addr: redisAddress,
Password: "",
Copy link
Member

Choose a reason for hiding this comment

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

We should consider having a shared password mapped from argocd-secret.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. Added flag for db and env variable for password

client := redis.NewClient(&redis.Options{
Addr: redisAddress,
Password: "",
DB: 0,
Copy link
Member

Choose a reason for hiding this comment

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

I think redis DB should also be exposed as a command line flag.

}

func appManagedResourcesKey(appName string) string {
return fmt.Sprintf("app|managed-resources|%s", appName)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider prefixing something like a db "version" to the keys so that when we make changes to the responses, we bump the version and the cache is automatically invalidated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is already implemented in getItem/setItem methods. As we agreed offline switched to separete const for cache version.

@alexmt alexmt merged commit cb9eb0a into argoproj:master Feb 13, 2019
@alexmt alexmt deleted the 937-redis-cache branch February 13, 2019 23:20
@alexmt alexmt restored the 937-redis-cache branch February 13, 2019 23:24
@alexmt alexmt deleted the 937-redis-cache branch July 15, 2019 19:52
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.

3 participants