Skip to content

Remove WATCH from Transaction #2033

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 6, 2021
Merged

Remove WATCH from Transaction #2033

merged 7 commits into from
Apr 6, 2021

Conversation

@sazzad16
Copy link
Contributor

Redis doesn't support WATCH within MULTI

Resolves #2030

@sazzad16 sazzad16 added this to the 4.0.0 milestone Jul 22, 2019
@sazzad16 sazzad16 requested a review from gkorland July 22, 2019 05:47
@gkorland
Copy link
Contributor

@sazzad16
LGTM!
But I would have add a test for pipeline.unwatch()

@gkorland gkorland mentioned this pull request Jul 22, 2019
Merged
@sazzad16
Copy link
Contributor Author

@gkorland Sure thing! See 251b532 in #2032

Please consider that it's very difficult to have proper test for unwatch in pipeline mode.

@gkorland
Copy link
Contributor

You can test unwatch with two parallel connections or using JedisPool

@sazzad16 sazzad16 mentioned this pull request Oct 29, 2019
Merged
@sazzad16
Copy link
Contributor Author

@gkorland I have added tests for WATCH and UNWATCH in #2086

https://github.com/xetorthio/jedis/blob/9de696236f59c4500f0b92c1b877d23b30e33e57/src/test/java/redis/clients/jedis/tests/PipeliningTest.java#L297-L346

so that these tests are valid for both 3.2+ and 4.0+ as this PR is for 4.0+ only.

@sazzad16
Copy link
Contributor Author

sazzad16 commented Nov 16, 2019 •
edited
Loading

Moved test for UNWATCH within MULTI in #2086 to be available for 3.2+

https://github.com/xetorthio/jedis/blob/4a1d06b869948527a560e6a3b0eb4ca8c2a293d8/src/test/java/redis/clients/jedis/tests/commands/TransactionCommandsTest.java#L400-L414

sazzad16 added a commit that referenced this pull request Nov 25, 2019
@sazzad16
Merge pull request #2086 from sazzad16/pipe-tx-tests
033ad67 
Tests for watch and unwatch

- Test: Redis supports UNWATCH within MULTI
- Tests for watch and unwatch while pipelining
- Fix test for brpoplpush

References: 
- #2033 (comment) 
- #2033 (comment) 
- #2033 (comment)
@sazzad16 sazzad16 removed the request for review from gkorland May 4, 2020 04:24
gkorland
gkorland previously approved these changes Jan 13, 2021
View reviewed changes
yangbodong22011
yangbodong22011 approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@yangbodong22011 yangbodong22011 left a comment

Choose a reason for hiding this comment

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

LGTM

@sazzad16 sazzad16 mentioned this pull request Jan 30, 2021
Merged
yangbodong22011
yangbodong22011 previously approved these changes Feb 7, 2021
View reviewed changes
@sazzad16 sazzad16 deleted the watch-in-multi branch February 7, 2021 09:33
@sazzad16 sazzad16 restored the watch-in-multi branch March 31, 2021 07:03
@sazzad16 sazzad16 reopened this Mar 31, 2021
@sazzad16 sazzad16 dismissed stale reviews from yangbodong22011 and gkorland via 4dab6f1 March 31, 2021 14:47
@sazzad16 sazzad16 requested a review from dengliming April 6, 2021 13:20
dengliming
dengliming approved these changes Apr 6, 2021
View reviewed changes
Copy link

@dengliming dengliming left a comment

Choose a reason for hiding this comment

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

LGTM

@sazzad16 sazzad16 merged commit b2e2cc5 into redis:master Apr 6, 2021
@sazzad16 sazzad16 deleted the watch-in-multi branch April 6, 2021 15:17
@sazzad16
Copy link
Contributor Author

More references:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@gkorland gkorland gkorland approved these changes

@dengliming dengliming dengliming approved these changes

@yangbodong22011 yangbodong22011 yangbodong22011 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

ClassCastException when transaction.watch(...) is used

4 participants

@sazzad16 @gkorland @dengliming @yangbodong22011