-
Notifications
You must be signed in to change notification settings - Fork 1
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
【腾讯犀牛鸟开源课题实战】对接mysql sdk #1
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,5 @@ | |||
build --cxxopt="--std=c++17" | |||
# build --copt=-O2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.bazelrc里,只需要留下面的选项就好
build --cxxopt="--std=c++17"
build --copt=-O2
BUILD
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个文件应该不需要,可以去掉
# | ||
# Tencent is pleased to support the open source community by making tRPC available. | ||
# | ||
# Copyright (C) 2023 THL A29 Limited, a Tencent company. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2024可以统一改下
char_set: "utf8mb4" | ||
num_shard_group: 4 | ||
thread_num: 4 | ||
thread_bind_core: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
绑核能设置目标核心号吗?
endif() | ||
|
||
string(REGEX REPLACE "^([0-9]+\\.[0-9]+)\\..*" "\\1" MYSQLCLIENT_MAJOR_VER "${MYSQLCLIENT_VERSION_TAG}") | ||
set(MYSQLCLIENT_URL "https://dev.mysql.com/get/Downloads/MySQL-${MYSQLCLIENT_MAJOR_VER}/mysql-${MYSQLCLIENT_VERSION_TAG}-linux-glibc2.17-x86_64-minimal.tar.xz") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
高版本的gcc,比如gcc12,能正常编译mysqlclient吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个不包含源码,是编译好的版本,因为源码那个仓库太大了,所有在官网找了这个minimal的发布的版本。只是链接的话gcc11是没问题的
) | ||
|
||
target_link_libraries(mysqlclient INTERFACE | ||
"${MYSQLCLIENT_LIB_DIR}/private/libcrypto.so" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ldd命令看下最后构建出二进制程序的so库路径,感觉这两so库得随着二进制发布而发布,程序分发到容器比较麻烦,能不能直接用系统的ssl库?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个如果找不到这个特定路径里的so会自动去找系统目录里的,应该没问题
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那这里可以直接用系统库的,框架环境也是要求安装openssl的
|
||
int main(int argc, char* argv[]) { | ||
ParseClientConfig(argc, argv); | ||
std::cout << "************************************\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
插件初始化可以放这里,配置解析不需要做插件初始化的事情,保持函数职责的单一性
#include <iostream> | ||
#include <memory> | ||
#include <string> | ||
#include "trpc/util/string_util.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不同头文件区块间应该加空格,头文件之间的排布应该是类似这样的,下面以dir2/foo2.cc的头文件引入为例子。
dir2/foo2.h(强关联的头文件,一般.cc文件里写对应.h文件,.h文件里写继承的父类等)
空行
C 系统头文件
空行
C++ 标准库头文件
空行
其他库的 .h文件
(可选)空行
本项目内 .h文件
trpc::Status s = proxy->Query(ctx, res, "select id, username from users where id = ? and username = ?", 3, "carol"); | ||
|
||
MYSQL_ERROR_CHECK(s, res); | ||
// const std::vector<std::tuple<int, std::string>>& res_set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果无法从上下推断出res_set的类型,则不应该使用auto,应该使用类型名,此处直接使用类型名就好了,无需增加注释,代码应该有自解释性
std::cout << "do something\n"; | ||
trpc::future::BlockingGet(std::move(future)); | ||
|
||
if(future.IsFailed()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future在106行被move了,这里用future对象会出现未定义行为,可以在第98行,判断future不为ready打印出错误的信息
|
||
DEFINE_string(client_config, "fiber_client_client_config.yaml", "trpc cpp framework client_config file"); | ||
|
||
void printResult(const std::vector<std::tuple<int, std::string>>& res_data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
大写 PrintResult,保持方法名风格的一致性
|
||
|
||
void TestAsyncTx(std::shared_ptr<trpc::mysql::MysqlServiceProxy>& proxy) { | ||
MysqlResults<OnlyExec> exec_res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删掉未使用的变量
proxy->Query(ctx, query_res, "select * from users"); | ||
|
||
// Do two query separately in the same one transaction and the handle will be moved to handle2 | ||
auto fu = proxy->AsyncBegin(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
命名保持一致,fut
PrintResultTable(res); | ||
std::cout << "\n\n"; | ||
|
||
return proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
例子里,也可以体现一下,执行失败调用rollback
} | ||
|
||
std::cout << "\n\n\n"; | ||
PrintResultTable(res2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res2的结果打印逻辑只保留一个就好了,不需要重复打印
ctx = trpc::MakeClientContext(proxy); | ||
s = proxy->Execute(ctx, exec_res, "delete from users where username = \"jack\""); | ||
MYSQL_ERROR_CHECK(s, exec_res); | ||
if(1 == exec_res.GetAffectedRowNum()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可以用 TRPC_ASSERT(exec_res.GetAffectedRowNum() == 1)
|
||
std::vector<std::vector<uint8_t>> null_flags; | ||
|
||
std::string error_message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private成员定义成 _ 结尾的命名
for(i = 1; i < failed_index.size(); i++) | ||
error.append(", ").append(fields_meta[failed_index[i]].name); | ||
error.append(")."); | ||
error_message.append(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
封装成工具函数可以返回Status代表成功和失败,也能记录错误信息
|
||
template <typename... Args> | ||
void MysqlResults<Args...>::SetRawMysqlRes(MYSQL_RES* res) { | ||
mysql_res_ = res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果 mysql_res_ 不为空,是不是还需要释放?如果不需要的话,注释说明下,这个赋值只会发生一次
|
||
template <typename... Args> | ||
bool MysqlResults<Args...>::IsValueNull(size_t row_index, size_t col_index) const { | ||
return null_flags[row_index][col_index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
需要做row_index和col_index的合法性校验?
|
||
template <typename... Args> | ||
bool MysqlResults<Args...>::OK() const { | ||
return error_message.empty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
换成错误码的设计吧,可以用trpc的Status来同时存错误码和错误消息
bool MysqlStatement::Init(const std::string& sql) { | ||
mysql_stmt_ = mysql_stmt_init(mysql_); | ||
if (mysql_stmt_ == nullptr) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
错误时需要打印错误日志,方便排查问题
|
||
std::string MysqlStatement::GetErrorMessage() { | ||
if(mysql_stmt_ != nullptr) | ||
return std::string(mysql_stmt_error(mysql_stmt_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql的api应该能获取错误码的
} | ||
|
||
bool MysqlStatement::BindParam(std::vector<MYSQL_BIND> &bind_list) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
去掉不必要的空格
class MysqlStatement { | ||
public: | ||
MysqlStatement(MYSQL* conn); | ||
~MysqlStatement() { assert(mysql_stmt_ == nullptr); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRPC_ASSERT
|
||
unsigned long GetParamsCount() { return params_count_; } | ||
|
||
MYSQL_RES* GetResultsMeta(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里是不是封装一下,只需要提供上层所用的必要信息?
}; | ||
|
||
|
||
class ExecuteStatus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可以复用trpc的Status
|
||
MysqlTime &MysqlTime::SetMonth(unsigned int month) { | ||
if(month > 0 && month <= 12) | ||
mt.month = month; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
设置失败时,打印一下错误日志
|
||
enum_mysql_timestamp_type MysqlTime::GetTimeType() const { return mt.time_type; } | ||
|
||
//std::string MysqlTime::ToString() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删掉无用注释
mt.day, mt.hour, mt.minute, mt.second); | ||
} | ||
|
||
void MysqlTime::FromString(const std::string &timeStr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
缺少非法性检查,可以注释加上时间字符串格式,以及合法性需要由用户保证
const char *MysqlTime::DataConstPtr() const { | ||
return reinterpret_cast<const char *>(&mt); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format可以统一格式化一下
return data_ == other.data_; | ||
} | ||
|
||
const char *MysqlBlob::data_ptr() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
接口定义保持统一,可以用DataConstPtr,和MysqlTime风格保持统一
return data_.data(); | ||
} | ||
|
||
size_t MysqlBlob::size() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
大写Size
const char* DataConstPtr() const; | ||
|
||
private: | ||
MYSQL_TIME mt{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mt_
|
||
|
||
|
||
class MysqlExecutorPoolImpl : public MysqlExecutorPool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
看上去只有一种 MysqlExecutorPool的实现,不用搞继承,除非有多种MysqlExectuorPool模式实现,需要通过多态来针对不同配置/场景、切换不同的实现模块
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
类名保持 MysqlExecturoPool 就好了
bool IsIdleTimeout(RefPtr<MysqlExecutor> executor); | ||
|
||
private: | ||
std::string m_ip_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
过多的类成员,需要进一步封装成结构体
std::atomic<uint32_t> executor_num_{0}; | ||
|
||
// The maximum number of connections that can be stored per `Shard` in `conn_shards_` | ||
uint32_t max_num_per_shard_{0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不同的成员有不同的含义,通过不同的结构体封装,保持对象定义职责的单一性
|
||
MysqlExecutorPool* MysqlExecutorPoolManager::CreateExecutorPool(const NodeAddr& node_addr) { | ||
MysqlExecutorPool* new_pool{nullptr}; | ||
new_pool = new MysqlExecutorPoolImpl(option_, node_addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
用智能指针而不是new
RefPtr<MysqlExecutor> idle_executor{nullptr}; | ||
|
||
uint32_t shard_id = shard_id_gen_.fetch_add(1); | ||
int retry_num = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
魔数,定义成变量名有意义的常量
|
||
namespace trpc::mysql { | ||
|
||
bool InitPlugin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不需要实现codec的话,这里也可以去掉
|
||
namespace trpc::mysql { | ||
|
||
MysqlServiceProxy::MysqlServiceProxy() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
构造函数使用默认的话,接口声明后面加上 =default就好了。
public: | ||
explicit MysqlExecutorPoolManager(MysqlExecutorPoolOption&& option); | ||
|
||
// MysqlExecutorPtr GetExecutor(const std::string& ip_port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
去掉无用注释
pool_option.max_idle_time = option->idle_time; | ||
pool_option.num_shard_group = mysql_conf_.num_shard_group; | ||
pool_option.char_set = mysql_conf_.char_set; | ||
pool_manager_ = std::make_unique<MysqlExecutorPoolManager>(std::move(pool_option)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里类之间关系好像不太对
类关系应该需要符合下面的才对
ExecutorPool(一)<->连接(多)
Manager(一)<->ExecutorPool(多)
ServiecProxy(一)<->ExectuorPool(一)
这样的话,Manager可以声明设计成单例模式,如果程序涉及Manager的Init和Destroy,可以单独提供两个接口,让用户在程序启动时初始化环境、在程序退出时销毁环境。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
哦,看错了,这里没问题,可以注释说下,pool_manager_是应对路由插件返回多个mysqlserver Ip的场景
|
||
if(!res.OK()) { | ||
Status status = kUnknownErrorStatus; | ||
status.SetErrorMessage(res.GetErrorMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
把错误码也关联到这里
|
||
private: | ||
std::unique_ptr<ThreadPool> thread_pool_{nullptr}; | ||
std::unique_ptr<MysqlExecutorPoolManager> pool_manager_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
声明时候可以置空
|
||
int filter_ret = RunFilters(FilterPoint::CLIENT_PRE_SEND_MSG, context); | ||
|
||
if(filter_ret == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
判断失败时直接返回,成功不用写在if块里
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
减少圈复杂度
related issue
mysql service相关主体部分