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(rest-datasource): support redis cluster and sentinel #1770

Merged

Conversation

eberhara
Copy link
Contributor

@eberhara eberhara commented Oct 4, 2018

This PR adds a new package called apollo-server-cache-redis-cluster that supports Redis Cluster and Sentinels on data sources caching.

Fixes #1750 and fixes #1725.

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@apollo-cla
Copy link

@eberhara: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Oct 4, 2018
@eberhara eberhara force-pushed the feat/add-redis-cluster-cache branch 2 times, most recently from 1db59d9 to c8b4a2d Compare October 4, 2018 19:45
@eberhara eberhara force-pushed the feat/add-redis-cluster-cache branch 4 times, most recently from 198ee4a to 8241c81 Compare October 4, 2018 21:12
@codecov-io
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

Merging #1770 into master will decrease coverage by 14.94%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1770       +/-   ##
===========================================
- Coverage   89.67%   74.72%   -14.95%     
===========================================
  Files           5       32       +27     
  Lines         184     1203     +1019     
  Branches       38      291      +253     
===========================================
+ Hits          165      899      +734     
- Misses         18      294      +276     
- Partials        1       10        +9
Impacted Files Coverage Δ
...ges/apollo-server-cache-redis-cluster/src/index.ts 100% <100%> (ø)
...erver-cache-redis-cluster/src/RedisClusterCache.ts 88.88% <88.88%> (ø)
...pollo-server-cache-redis-cluster/src/RedisCache.ts 93.75% <93.75%> (ø)
...ackages/apollo-server-express/src/expressApollo.ts 86.36% <0%> (ø) ⬆️
packages/apollo-server-express/src/index.ts 100% <0%> (ø) ⬆️
packages/apollo-server-koa/src/koaApollo.ts 80% <0%> (ø) ⬆️
...kages/apollo-datasource-rest/src/RESTDataSource.ts 86.41% <0%> (ø)
...s/apollo-server-cloud-function/src/ApolloServer.ts 43.63% <0%> (ø)
...ackages/apollo-server-cache-memcached/src/index.ts 100% <0%> (ø)
... and 23 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 a619a8c...b821d7b. Read the comment docs.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/apollo-server-cache-redis-cluster/src/index.ts Outdated Show resolved Hide resolved
packages/apollo-server-cache-redis-cluster/src/index.ts Outdated Show resolved Hide resolved
packages/apollo-server-cache-redis-cluster/src/index.ts Outdated Show resolved Hide resolved
@eberhara eberhara force-pushed the feat/add-redis-cluster-cache branch 5 times, most recently from 1dc5c14 to e00b347 Compare October 7, 2018 20:58
@eberhara eberhara force-pushed the feat/add-redis-cluster-cache branch 9 times, most recently from aec04cf to 318c871 Compare October 20, 2018 18:36
@eberhara
Copy link
Contributor Author

Just rebased the branch again so someone can review it. ✅

@veeramarni
Copy link

@shinyaohira wondering if you got any chance to review this PR?

@shinyaohira
Copy link
Contributor

@eberhara @veeramarni I've commented about some minor issues, documentation and typings. I think the code itself is ok 👍

@jacobhummel
Copy link

Would love to see some attention to this PR. It seems simple and well done by @eberhara. Since redis is so often used with clustering and sentinel, this would be a huge win for Apollo Server caching IMO.

On whether it should replace the existing module, I'm not sure. Since they have different features, I'd lean towards adding a new module, as @eberhara has suggested.

@ulrikstrid
Copy link
Contributor

@eberhara Would you consider creating a separate package for this? Or would you mind if someone else did?

@eberhara
Copy link
Contributor Author

hey @ulrikstrid .
I dont mind at all.

I actually believe a separate package is a more elegant solution. However I don't feel I'll have time next week to doing so.

Are you planning create the separate package?

@ulrikstrid
Copy link
Contributor

I vendored your code for now but can create a repo and publish to npm as it seems you're okay with it

@benoitemile
Copy link

@ulrikstrid could you please do it ? this pr seems to be a deadend, we'd really like to be able to use redis sentinel.

@ulrikstrid
Copy link
Contributor

@benoitemile I'm working on it right now, Will report back here when it's done.

@veeramarni
Copy link

Just checking if anyone has created a package for it? Either way please let us know.

@ulrikstrid
Copy link
Contributor

I've created this repo https://github.com/XenitAB/apollo-server-cache-redisio but have not had time to finish everything yet

abernix added 3 commits May 23, 2019 18:41
We're doing a major version bump here, so I'm going to take the opportunity
to "drop" Node.js 6 support.  Granted, this `engines` field is only a
suggestion that is loosely enforced by some tools (and not obeyed by Node.js
or npm), it's the spirit of the thought that counts, I think? :)
@ulrikstrid
Copy link
Contributor

@abernix does this merge mean that it's going to be moved into master?

@abernix
Copy link
Member

abernix commented May 23, 2019

Sorry for the long delay here. I've merged in master and manually applied the various changes which have happened on the main trunk (for example, changes around the KeyValueCache and its typings) over the last many, many months.

Since this is a breaking change (I think?), if someone can make sure that there's some brief/necessary/clear instructions added to the CHANGELOG.md as to what existing users would need to do to upgrade, I think we can get this released. Either an additional commit added to this PR (would have to be by @eberhara), or a PR onto this PR (by anyone else) would be most welcomed!

Thanks!

@eberhara
Copy link
Contributor Author

hey @abernix ! Just added the instructions to the Changelog!

@abernix abernix changed the base branch from master to release-2.6.0 May 24, 2019 14:50
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this, and apologies for how long it took to land. The plan is to publish this as an alpha from the release-2.6.0 branch pretty soon (and I've updated the target branch for this PR to release-2.6.0 accordingly).

@abernix abernix merged commit 7667cd4 into apollographql:release-2.6.0 May 24, 2019
@abernix abernix added this to the Release 2.6.0 milestone May 24, 2019
@eberhara eberhara deleted the feat/add-redis-cluster-cache branch May 24, 2019 18:48
@eberhara
Copy link
Contributor Author

These are great news @abernix !
Thanks!!! ❤️

abernix added a commit that referenced this pull request May 24, 2019
@abernix
Copy link
Member

abernix commented May 24, 2019

I've published apollo-server-cache-redis@1.0.0-alpha.6 as part of the 2.6.0 release PR. Please try it out and provide confirmation that it works! 😄

Thanks again for working on this!

@kbariotis
Copy link

@eberhara @shinyaohira I understand this is an old PR ofc, but just out of curiosity, what exactly does DataLoader solves here? Thank you

@eberhara
Copy link
Contributor Author

@kbariotis it combines multiple get trips to redis into a single mget one - that's very powerful for queries that run more than one get in parallel.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.