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

Consider Supporting Redis with CRaC #63

Closed
charlie-harvey opened this issue Jan 25, 2023 · 7 comments · Fixed by #78
Closed

Consider Supporting Redis with CRaC #63

charlie-harvey opened this issue Jan 25, 2023 · 7 comments · Fixed by #78
Assignees
Milestone

Comments

@charlie-harvey
Copy link

Feature description

import io.lettuce.core.RedisClient
import io.micronaut.crac.OrderedResource
import jakarta.inject.Named
import jakarta.inject.Singleton
import mu.KotlinLogging
import org.crac.Context
import org.crac.Resource

private val logger = KotlinLogging.logger {}

@Singleton
class RedisResource(
    @Named("redisClientA") private val redisClientA: RedisClient,
    @Named("redisClientB") private val redisClientB: RedisClient
) : OrderedResource {

    override fun beforeCheckpoint(context: Context<out Resource>?) {
        logger.info { "stopping redis clients before checkpoint" }
        redisClientA.shutdown()
        redisClientB.shutdown()
        logger.info { "done stopping redis clients before checkpoint" }
    }

    override fun afterRestore(context: Context<out Resource>?) {
        logger.info { "starting redis clients after restore (just logging for now)" }
        logger.info { "done starting redis clients after restore (just logging for now)" }
    }
}

This is what I have. But I'm really not sure if its right. I looked at the Hikari one but the RedisClient doesn't have as many methods. Just .shutdown() and .connect(). And .connect() in the afterRestore() doesn't do what I expect. I think the client is already started and connected so the .connect() fails.

Do I actually even need this OrderedResource? I'm not sure. Do the clients need to be shut down? Does someone else know more about this than I do? I hope so.

Thanks!

@timyates
Copy link
Contributor

timyates commented Mar 9, 2023

Hiya!

I'm working on a more general fix for Redis over here #78

The clients need to be closed, so that the checkpoint can be successfully taken, however as you've seen there's no way of "pausing" and "resuming" connections to Redis (which are StatefulRedisConnection objects based on Netty)

So in the above PR, what I'm doing is destroying the beans before the checkpoint. Then, so long as the Service which makes use of the beans is @Refreshable, the client should be re-created on restore.

It's working under test, and I'm now checking that it also works in a real application

/me brushes dust off his x86 machine

@timyates timyates linked a pull request Mar 13, 2023 that will close this issue
@timyates
Copy link
Contributor

Fixed by #78 and released in v1.2.1

@timyates timyates added this to the 1.2.1 milestone Mar 23, 2023
@rjaros87
Copy link

rjaros87 commented Apr 3, 2023

Hi @timyates,
Does your solution also support StatefulRedisPubSubConnection ?

For StatefulRedisConnection & @Refreshable annotation it works, but StatefulRedisPubSubConnection connection for me is still closed :/

timyates added a commit that referenced this issue Apr 11, 2023
As raised here #63 (comment)

StatefulRedisPubSubConnection beans were not destroyed during checkpointing, and so we were getting connection closed errors.
@timyates
Copy link
Contributor

Hi @rjaros87 thanks for the report!

This should fix it: #91

sdelamo pushed a commit that referenced this issue Apr 11, 2023
* Also destroy StatefulRedisPubSubConnectionResource beans

As raised here #63 (comment)

StatefulRedisPubSubConnection beans were not destroyed during checkpointing, and so we were getting connection closed errors.

* Fix. There are no PubSub beans, they are all connection beans

* Reduce duplication

* Checkstyle...

* Naming

* Visibility
@rjaros87
Copy link

I've created separate issue for that #89. I will check it later and close.

sdelamo added a commit that referenced this issue Jun 1, 2023
* use Gradle Kotlin DSL

* Update slsa-framework/slsa-github-generator action to v1.3.0 (#42)

* ci: distribution temurin cla provenance (#44)

* Update stefanzweifel/git-auto-commit-action action to v4.15.4 (#43)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* build: Micronaut Framework 3.7.4

* build: spock 2.2-groovy-3.0

* doc: eager singletons and inject HttpClient (#66)

See micronaut-projects/micronaut-test#716

* ci: projectVersion 1.2.0-SNAPSHOT

* ci: githubCoreBranch 3.9.x

* build: Micronaut Framework 3.8.5

* build: Micronaut Test 3.8.2

* build: Spock 2.3-groovy-3.0

* wip DataSourceResolver

* Convert Delegating datasource and filter non-hikari contents

* Test

* Test to ensure DBCP still works and isn't supported

* Try to downgrade Jooq

* remove @Inject BeanContext ctx

* Simplify composite (#73)

* Simplify composite

* Extract common code to method

---------

Co-authored-by: Tim Yates <[email protected]>

* build: Micronaut Framework 3.8.6

* build: Micronaut Test 3.9.1

* ci: GitHub Actions sync

* [skip ci] Release v1.2.0

* Back to 1.2.1-SNAPSHOT

* Redis support for CRaC (#78)

* wip: Redis support for CRaC

- Currently doesn't work for `@Cacheable` tags
- Needs manual testing with a proper app

* Add base buildSrc plugin

* Add config and tests

* Docs

* Update src/main/docs/guide/resource/redis.adoc

Co-authored-by: Sergio del Amo <[email protected]>

---------

Co-authored-by: Sergio del Amo <[email protected]>

* [skip ci] Release v1.2.1

* Back to 1.2.2-SNAPSHOT

* bug: StatefulRedisPubSubConnection beans are not supported (#91)

* Also destroy StatefulRedisPubSubConnectionResource beans

As raised here #63 (comment)

StatefulRedisPubSubConnection beans were not destroyed during checkpointing, and so we were getting connection closed errors.

* Fix. There are no PubSub beans, they are all connection beans

* Reduce duplication

* Checkstyle...

* Naming

* Visibility

* [skip ci] Release v1.2.2

* Back to 1.2.3-SNAPSHOT

* Handle multiple named Redis servers (#92)

* Handle multiple named Redis servers

* Remove debug

* Fix imports

* Remove deprecated experimental classes and add accepted-api-changes

* [skip ci] Release v1.2.3

* Back to 1.2.4-SNAPSHOT

* Document checkpoint simulator, with java/groovy/kotlin examples (#117)

* Document checkpoint simulator, with java/groovy/kotlin examples

closes #115

* documentation for CheckpointSimulator

* Java/Groovy/Kotlin docs examples

* Extract documented classes to get rid of static

---------

Co-authored-by: Tim Yates <[email protected]>

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: micronaut-build <[email protected]>
Co-authored-by: Tim Yates <[email protected]>
Co-authored-by: micronaut-build <[email protected]>
Co-authored-by: Dean Wette <[email protected]>
@fhoner
Copy link

fhoner commented Jul 19, 2024

Hi @timyates , is something similar planned for MongoDB?

@timyates
Copy link
Contributor

Can you raise an issue? And as always, PRs welcome 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants