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

feature: impl hello and set name command with resp2 compatible #1245

Merged
merged 3 commits into from
Feb 3, 2023

Conversation

weedge
Copy link
Contributor

@weedge weedge commented Feb 2, 2023

This patch tries to fix those issue:

  1. 使用pika出现不支持hello命令 #1241
  2. 支持client setname 命令 #1242

tips: this patch don't support RESP3 switch, maybe next feature to do :)

@@ -80,6 +80,9 @@ class PinkConn : public std::enable_shared_from_this<PinkConn> {
return is_reply_;
}

std::string get_name() { return name_; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

函数名为name() ,会更好一点。

return;
}

if (!strcasecmp(argv_[1].data(), "setname") && argv_.size() == 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

需要考虑下 处理setname foo bar dummy这种情况么

res_.SetRes(CmdRes::kErrOther, "-NOPROTO unsupported protocol version");
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

next_arg++;放在这里会更好一点,表示我们在这里处理完了一个参数。

}

for (; next_arg < argv_.size(); ++next_arg) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

直接next_arg= 2 更好一点?作为读者,我们不必关心next_arg前面的处理逻辑。

break;
default:
LOG(WARNING) << "unknown role" << host_role;
Copy link
Collaborator

Choose a reason for hiding this comment

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

更多的信息能帮助定位哪个客户端发送的信息有误,例如connection 的ip 地址。
日志级别我任务info级别足够了,因为不是pika server 本身的错误。用户设置高的日志级别就是为了过滤掉信息

INVALID_CONN,
};

static AuthResult AuthenticateUser(const std::string& pwd,
Copy link
Collaborator

Choose a reason for hiding this comment

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

考虑下如何跟auth cmd 复用,逻辑是相似的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

考虑到auth cmd是已有稳定代码

@kernelai kernelai changed the title impl hello cmd feature: impl hello and set name CMD with resp2 compatible Feb 3, 2023
@kernelai kernelai changed the title feature: impl hello and set name CMD with resp2 compatible feature: impl hello and set name command with resp2 compatible Feb 3, 2023
@kernelai kernelai merged commit fdaddd2 into OpenAtomFoundation:unstable Feb 3, 2023
@weedge weedge deleted the feature/hellocmd branch February 3, 2023 15:03
@kernelai kernelai mentioned this pull request Feb 6, 2023
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.

2 participants