-
-
Notifications
You must be signed in to change notification settings - Fork 50
Store current reference cache in redis #3582
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
| } | ||
|
|
||
| // This hostname has to match the redir service name in app.yml. | ||
| static let hostname = "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.
We could make this configurable but there's not much point as it's not really dependent on anything, just a static name defined in app.yml.
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 the only bit that I thought we might want to tweak and make into an environment variable, primarily so that a purely local development setup could reference LOCALHOST:6379 and had redis running in a container.
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've simply set 127.0.0.1 redis in my /etc/hosts locally but of course making in configurable is better.
| boundEventLoop: NIOSingletons.posixEventLoopGroup.any() | ||
| ) | ||
| self.client = try await connection.get() | ||
| } |
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.
Please give this whole actor Redis a thorough review, in particular how it's setting up the shared singleton with retries and the async throws initialiser that's using the @preconcurrency imported RedisConnection.
In particular, @preconcurrency is required, because RedisConnection isn't Sendable. I think this is correctly set up but good to have a proper review of this.
Unfortunately, there's not much I can see us do around this in terms of testing other than verifying that writing to and reading from Redis works on dev (which it does).
| @Dependency(\.redis) var redis | ||
| await redis.set(key: getKey(owner: owner, repository: repository), | ||
| value: reference, | ||
| expiresIn: timeToLive) |
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.
@heckj , this is how you'd interface with Redis once this lands. We wouldn't be using app.redis at all but rather the private Redis actor via the RedisClient dependency, replacing your PR #3580.
The only thing this doesn't cover is setup for local use of Redis, i.e. during development. It should be easy to add though, either by making the hostname configurable or just setting a redis alias in /etc/hosts on the local machine. The latter is certainly easiest and fastest to get going.
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.
Correction, I just realised I misread your PR #3580. I was focused on the part
- app.redis.configuration = try RedisConfiguration(hostname: "redis")
+ if let redisHost = Environment.get("REDIS_HOST") {
+ app.redis.configuration = try RedisConfiguration(hostname: redisHost)
+ } else {
+ app.logger.warning("REDIS_HOST not set, caching disabled.")
+ }but really the point of it is to add a bringup for a local Redis and the hostname config.
6d59b58 to
1e40561
Compare
This starts using Redis in place of the per node current reference cache for doc links.
I've ad hoc deployed this revision to dev and triggered the code path where it writes to Redis:
I'll update the PR now to replace the in-memory cache.
Visit https://staging.swiftpackageindex.com/SwiftPackageIndex/SemanticVersion/~/documentation/semanticversion to hit the cache code path.