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.

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

feat: let pika slave support Redis transaction #2441

Merged
merged 3 commits into from
Mar 13, 2024
Merged

feat: let pika slave support Redis transaction #2441

merged 3 commits into from
Mar 13, 2024

Conversation

happy-v587
Copy link
Contributor

修复 issuse：#2422

另外，测试发现 pika slave 执行 watch 后，与 redis 预期也不同。

  • redis slave上执行 watch 的测试结果
127.0.0.1:6378> get a
"a"
127.0.0.1:6378> watch a
OK
127.0.0.1:6378> multi
OK
127.0.0.1:6378> get a
QUEUED
127.0.0.1:6378> get b
QUEUED
127.0.0.1:6378> exec
(nil)
  • pika slave 上执行 watch 的测试结果
127.0.0.1:9222> get a
"ddd"
127.0.0.1:9222> watch a
OK
127.0.0.1:9222> multi
OK
127.0.0.1:9222> get a
QUEUED
127.0.0.1:9222> get b
QUEUED
127.0.0.1:9222> exec
1) "ddds"
2) (nil)

目前推测是主从复制时没有调用 SetTxnFailedFromKeys 导致，需要确认下这个问题是否需要在此pr一同修复？

@github-actions github-actions bot added the ☢️ Bug Something isn't working label Mar 3, 2024
@AlexStocks AlexStocks changed the title fix: pika slave 在执行 multi exec后，与 redis 预期不同 fix: let pika slave support Redis transaction Mar 3, 2024
@AlexStocks AlexStocks requested a review from chejinge March 3, 2024 13:39
@chejinge
Copy link
Collaborator

chejinge commented Mar 4, 2024

修复 issuse：#2422

另外，测试发现 pika slave 执行 watch 后，与 redis 预期也不同。

  • redis slave上执行 watch 的测试结果
127.0.0.1:6378> get a
"a"
127.0.0.1:6378> watch a
OK
127.0.0.1:6378> multi
OK
127.0.0.1:6378> get a
QUEUED
127.0.0.1:6378> get b
QUEUED
127.0.0.1:6378> exec
(nil)
  • pika slave 上执行 watch 的测试结果
127.0.0.1:9222> get a
"ddd"
127.0.0.1:9222> watch a
OK
127.0.0.1:9222> multi
OK
127.0.0.1:9222> get a
QUEUED
127.0.0.1:9222> get b
QUEUED
127.0.0.1:9222> exec
1) "ddds"
2) (nil)

目前推测是主从复制时没有调用 SetTxnFailedFromKeys 导致，需要确认下这个问题是否需要在此pr一同修复？

辛苦一并修复下，感谢啦

@happy-v587
Copy link
Contributor Author

happy-v587 commented Mar 5, 2024 •
edited
Loading

稍后进行修复~

@happy-v587 happy-v587 force-pushed the unstable branch 2 times, most recently from 40d866d to 5545593 Compare March 6, 2024 15:08
@happy-v587
Copy link
Contributor Author

@chejinge PTAL

chejinge
chejinge reviewed Mar 7, 2024
View reviewed changes
src/pika_repl_bgworker.cc Outdated
}
involved_conns = dispatcher->GetInvolvedTxn(table_keys);
for (auto& conn : involved_conns) {
if (auto c = std::dynamic_pointer_cast<PikaClientConn>(conn); c != nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个函数建议稍微做下代码优化，感觉这个逻辑好乱 好多if else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已优化

@chejinge
Copy link
Collaborator

chejinge commented Mar 8, 2024

image
试了一下好像逻辑不太对

@happy-v587
Copy link
Contributor Author

在执行exec时报错。

如下是redis-server行为
image

@happy-v587
fix: pika slave support multi/exec cmd
6b7aa37 
OpenAtomFoundation#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>
@happy-v587
fix: pika slave support watch cmd
d700f17 
OpenAtomFoundation#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>
@happy-v587
test: add replication-test-go, let slave support watch/unwatch/multi/…
34ee121 
…exec/discard

Signed-off-by: HappyUncle <code4happy@gmail.com>
@happy-v587
Copy link
Contributor Author

happy-v587 commented Mar 13, 2024 •
edited
Loading

新增了ut @chejinge @AlexStocks PTAL

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.

Added ut @chejinge PTAL

@AlexStocks AlexStocks merged commit 43606e5 into OpenAtomFoundation:unstable Mar 13, 2024
12 checks passed
@AlexStocks AlexStocks changed the title fix: let pika slave support Redis transaction feat: let pika slave support Redis transaction Mar 21, 2024
cheniujh added a commit to cheniujh/pika that referenced this pull request May 8, 2024
@cheniujh
Revert "fix: let pika slave support Redis transaction (OpenAtomFounda…
bed769f 
…tion#2441)"

This reverts commit 43606e5
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
@happy-v587
fix: let pika slave support Redis transaction (OpenAtomFoundation#2441)
32a793b 
* fix: pika slave support multi/exec cmd

OpenAtomFoundation#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>

* fix: pika slave support watch cmd

OpenAtomFoundation#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>

* test: add replication-test-go, let slave support watch/unwatch/multi/exec/discard

Signed-off-by: HappyUncle <code4happy@gmail.com>

---------

Signed-off-by: HappyUncle <code4happy@gmail.com>
chejinge pushed a commit that referenced this pull request Aug 14, 2024
@happy-v587 @brother-jin
fix: let pika slave support Redis transaction (#2441)
0aaa704 
* fix: pika slave support multi/exec cmd

#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>

* fix: pika slave support watch cmd

#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>

* test: add replication-test-go, let slave support watch/unwatch/multi/exec/discard

Signed-off-by: HappyUncle <code4happy@gmail.com>

---------

Signed-off-by: HappyUncle <code4happy@gmail.com>
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
@happy-v587
fix: let pika slave support Redis transaction (OpenAtomFoundation#2441)
5f6d2b2 
* fix: pika slave support multi/exec cmd

OpenAtomFoundation#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>

* fix: pika slave support watch cmd

OpenAtomFoundation#2422
Signed-off-by: HappyUncle <code4happy@gmail.com>

* test: add replication-test-go, let slave support watch/unwatch/multi/exec/discard

Signed-off-by: HappyUncle <code4happy@gmail.com>

---------

Signed-off-by: HappyUncle <code4happy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chejinge chejinge chejinge approved these changes

@AlexStocks AlexStocks AlexStocks approved these changes

Assignees
No one assigned
Labels
3.5.3 ☢️ Bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@happy-v587 @chejinge @Issues-translate-bot @AlexStocks