Skip to content

Conversation

@BusyByte
Copy link
Contributor

@BusyByte BusyByte commented Dec 8, 2023

Closes: #822

@BusyByte
Copy link
Contributor Author

BusyByte commented Dec 8, 2023

@gvolpe and @decoursin can you please review?

@decoursin
Copy link

Sorry, I'm not going to be able to review, it's a little bit beyond my skill level to grasp the codebase in a reasonable time to give a qualified review. I do appreciate your potential contributions though.

Copy link
Member

@gvolpe gvolpe left a comment

Choose a reason for hiding this comment

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

Hey sorry for taking so long to review this, I've been away the last two months. Looking good in general, just left a few remarks.

Question for you: you mentioned you're using this approach in production, how is it working for you?

Thanks a lot for your contribution! 💟

@andriibutko
Copy link
Collaborator

andriibutko commented Mar 11, 2024

@BusyByte do you need a help with fixes?

….scala

Co-authored-by: Gabriel Volpe <volpegabriel@gmail.com>
@BusyByte
Copy link
Contributor Author

@gvolpe @yourzbuddha just getting back to this. kind of fell off my radar for a bit. I can see if I can get a chance to look at it today.

@andriibutko
Copy link
Collaborator

andriibutko commented Mar 21, 2024

@BusyByte I can assist.

@BusyByte
Copy link
Contributor Author

@gvolpe let me know if you need any additional changes from me

@gvolpe
Copy link
Member

gvolpe commented Mar 21, 2024

@BusyByte thanks!

I'm not sure if adding a private function to MkRedis is actually backwards compatible.

Yeah I've no idea either. Ideally, we should use MiMa, if anyone volunteers to do the work :)

In the meantime, it is clarified in the README file:

The API is quite stable and heavily used in production. However, binary compatibility is not guaranteed across versions for now.

Copy link
Member

@gvolpe gvolpe 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!

@gvolpe gvolpe merged commit b0d60d8 into profunktor:series/1.x Mar 21, 2024
@BusyByte
Copy link
Contributor Author

@yourzbuddha thanks for offering to help, I had time today so just knocked it out

@decoursin
Copy link

Thanks guys :)

libraryDependencies ++= Seq(
Libraries.catsEffectKernel,
Libraries.redisClient,
Libraries.keyPool % Optional,
Copy link
Member

Choose a reason for hiding this comment

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

hey @BusyByte, is there any particular reason this is added as Optional? It's a bit of a footgun, looks like we've just spotted it causing trouble in: #955

How about we just move the dependency to the effects module, like this: #956 ?

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.

Feature Request: Connection Pooling

5 participants