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

rproxy: add incr/incrby/decr/decrby #146

Merged
merged 22 commits into from
Jul 29, 2018
Merged

Conversation

acelyc111
Copy link
Member

No description provided.

Copy link
Contributor

@shengofsun shengofsun left a comment

Choose a reason for hiding this comment

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

有一个一堆rpc_code的地方,应该是proxy_layer.cpp下,把INC, CHECK_AND_SET这些code的allow inline都设置为1

qinzuoyan
qinzuoyan previously approved these changes Jul 27, 2018
@@ -938,6 +942,104 @@ void redis_parser::geo_radius_by_member(message_entry &entry)
hash_key, "", radius_m, count, sort_type, 2000, search_callback);
}

void redis_parser::incr(message_entry &entry) { counter_internal(entry); }
Copy link
Contributor

@neverchanje neverchanje Jul 27, 2018

Choose a reason for hiding this comment

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

補充一下注釋?像其他函數一樣。

Copy link
Member Author

Choose a reason for hiding this comment

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

这里不用加注释,因为函数名本身就是自解释的。其他有的加了注释是因为跟redis原生命令有些出入

return;
}
} else {
dassert(false, "command not support");

This comment was marked as resolved.

@acelyc111
Copy link
Member Author

@shengofsun INC已添加, CHECK_AND_SET目前rproxy还没使用,暂时不加吧

qinzuoyan
qinzuoyan previously approved these changes Jul 27, 2018
neverchanje
neverchanje previously approved these changes Jul 27, 2018
Copy link
Contributor

@neverchanje neverchanje left a comment

Choose a reason for hiding this comment

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

LGTM

qinzuoyan
qinzuoyan previously approved these changes Jul 27, 2018
@@ -35,6 +35,7 @@ proxy_stub::proxy_stub(const proxy_session::factory &f,
dsn::task_spec::get(dsn::apps::RPC_RRDB_RRDB_GET_SCANNER_ACK)->allow_inline = true;
dsn::task_spec::get(dsn::apps::RPC_RRDB_RRDB_SCAN_ACK)->allow_inline = true;
dsn::task_spec::get(dsn::apps::RPC_RRDB_RRDB_CLEAR_SCANNER_ACK)->allow_inline = true;
dsn::task_spec::get(dsn::apps::RPC_RRDB_RRDB_INCR)->allow_inline = true;

This comment was marked as resolved.

Copy link
Contributor

@shengofsun shengofsun left a comment

Choose a reason for hiding this comment

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

相应的单元测试有加吗?貌似没看到

@acelyc111
Copy link
Member Author

这里主要是看到proxy这块之前也没有针对接口的单测,而那个redis协议的python脚本基本也不起作用(都想删掉的),所以现在都还没加,包括之前的redis geo的单测。
我建个issue以后单独加吧

@acelyc111 acelyc111 dismissed stale reviews from qinzuoyan and neverchanje via 4b87b25 July 28, 2018 16:24
Copy link
Contributor

@shengofsun shengofsun left a comment

Choose a reason for hiding this comment

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

proxy.connection下的测试其实是对这些操作能work的一个基本测试。
anyway, 我先给你过了吧。后面你可以补一下这些新操作以及geo的测试。

@acelyc111 acelyc111 merged commit 2b92df0 into apache:master Jul 29, 2018
@neverchanje neverchanje added the component/rpoxy redis proxy for pegasus label Sep 13, 2018
neverchanje pushed a commit to neverchanje/pegasus that referenced this pull request Jul 13, 2019
* add incr/incrby/decr/decrby for redis proxy

Former-commit-id: caee216d1b3c842d377b0c6086c3afa9165ed8a8 [formerly 2b92df0]
Former-commit-id: a5b375e2b944e927eb9c771b935916c74328ac07
acelyc111 pushed a commit that referenced this pull request Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rpoxy redis proxy for pegasus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants