Skip to content

add lock blocking control to redis/valkey.#272

Closed
jvanasco wants to merge 6 commits intosqlalchemy:mainfrom
jvanasco:fix-redis_locking
Closed

add lock blocking control to redis/valkey.#272
jvanasco wants to merge 6 commits intosqlalchemy:mainfrom
jvanasco:fix-redis_locking

Conversation

@jvanasco
Copy link
Copy Markdown
Member

Fixes #271

This just accepts the Redis/Valkey .lock( blocking control arguments (blocking: bool; blocking_timeout: Union[int, float]) with a "lock_" prefix, then proxies them to the client as the correct kwarg when the lock is first created.

Change-Id: I680602a3abe8c9f185017c6d00b7b3c3420b3ec7

Change-Id: I680602a3abe8c9f185017c6d00b7b3c3420b3ec7
Change-Id: I4fcab991d6f1dbee2afe87af6e18b613ec8fa3f4
Change-Id: I97bae3c4707502347780349f198075d32a69ef84
Change-Id: I674e939448e4206387df3b33d2df2c462fdb2651
@jvanasco
Copy link
Copy Markdown
Member Author

Updated against main and with some comments. Can someone please take a look at this PR?

@zzzeek zzzeek requested a review from sqla-tester September 11, 2025 16:09
Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 3151d8a of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Copy Markdown
Collaborator

New Gerrit review created for change 3151d8a: https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

@zzzeek
Copy link
Copy Markdown
Member

zzzeek commented Sep 11, 2025

this looks fine. any appetite to also add #276 to this?  (if so, this would be a separate changelog file 276.rst)

@jvanasco
Copy link
Copy Markdown
Member Author

I'll get #276 done within the hour.

Change-Id: Ice0b1140f718780f4919c81e44ca058aade8111f
@jvanasco
Copy link
Copy Markdown
Member Author

In #bf8469a, I implemented #267 with ssl only - and using the docstring to point users to the connection_kwargs dict. There are currently 14 "ssl_" prefixed args; supporting them all directly did not seem like something worth implementing due to maintenance requirements – and the big issue seems to be documentation hinting users that connection_kwargs supports them.

@zzzeek zzzeek requested a review from sqla-tester September 11, 2025 18:34
Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision bf8469a of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Copy Markdown
Collaborator

Patchset bf8469a added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

Michael Bayer (zzzeek) wrote:

this is good just the docs need to use two backticks for words like

ssl

otherwise they come out as some weird italics or something

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

Comment thread dogpile/cache/backends/valkey.py Outdated
used to create the lock.

:param ssl: boolean, default ``None``. If set, this is passed to the
``valkey.StrictValkey`` constructor as `ssl`. All additional `ssl_`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Michael Bayer (zzzeek) wrote:

just an RST note, you need two backticks for code keywords:

``ssl``

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

Change-Id: I401294b568c8444e06df0208335140fc0c5bc263
@zzzeek zzzeek requested a review from sqla-tester September 12, 2025 15:55
Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 0b44a26 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Copy Markdown
Collaborator

Patchset 0b44a26 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

Jonathan Vanasco (jvanasco) wrote:

I've pushed a commit to github. Is there a way to get the bot to pull from it? I am a bit weary of Gerrit since accidentally pushing an unwanted commit into main a few years ago.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

used to create the lock.

:param ssl: boolean, default ``None``. If set, this is passed to the
``valkey.StrictValkey`` constructor as `ssl`. All additional ``ssl_``
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Jonathan Vanasco (jvanasco) wrote:

connection_kwargs too.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Michael Bayer (zzzeek) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

@sqla-tester
Copy link
Copy Markdown
Collaborator

Michael Bayer (zzzeek) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112

@sqla-tester
Copy link
Copy Markdown
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/6112 has been merged. Congratulations! :)

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.

redis backend should accept/persist a blocking_timeout for locks

3 participants