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

Feat: Add consistent hash and modulo hash load balance with their configuration #158

Merged
merged 36 commits into from
Sep 18, 2024

Conversation

tracer07
Copy link
Contributor

test file is in /trpc/naiming/direct/selector_hash_test.cc

Copy link

github-actions bot commented Jul 15, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@weimch
Copy link
Contributor

weimch commented Jul 15, 2024

.clangd文件夹的文件不需要包含进commit,可以删掉,要不是代码review非常卡

@tracer07
Copy link
Contributor Author

ok,已经移除.clangd文件夹

Copy link
Contributor

Choose a reason for hiding this comment

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

用于测试的文件不需要放到commit里

#include "examples/features/fiber_forward/proxy/forward.trpc.pb.h"
#include "examples/helloworld/helloworld.trpc.pb.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

不需要格式化除了你新增代码外的其他文件,避免代码review出现大量无关的内容,可以用根目录的.clang-format文件来格式化代码(trpc-cpp代码基本都已经基于此文件的规则格式化了)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,我已经恢复原来的格式,并再次提交了

external Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

删掉不需要提交的文件

Copy link
Contributor

Choose a reason for hiding this comment

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

这个文件也删掉,2w行太多了

.bazelrc Outdated
@@ -2,4 +2,6 @@ build --cxxopt="--std=c++17"
build --copt=-O2
#build --copt=-g --strip=never
build --jobs 16
build --linkopt=-lssl
build --linkopt=-lcrypto
Copy link
Contributor

Choose a reason for hiding this comment

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

为啥要加这两个编译选项呢?实现会用到ssl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是我本地跑run_example.sh的时候,会报找不到openssl库,所以加上的,如果不需要的话,我可以在提交的时候删除

Copy link
Contributor

Choose a reason for hiding this comment

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

可以删掉的

@@ -0,0 +1,37 @@
//-----------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

拷贝的文件需要在头部加上注释,声明来源,另外,hash功能不能通过库的形式引入吗?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

按照要求,我把不需要的文件删除,并且将hash功能从类中抽离出来了

@tracer07
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

liuzengh added a commit to trpc-group/cla-database that referenced this pull request Jul 16, 2024
@@ -0,0 +1,108 @@
//
Copy link
Contributor

Choose a reason for hiding this comment

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

可以保持trpc-cpp的单测风格,单测文件名,要待测文件名前缀一样,且放在同一个目录里,yaml文件可以放在testing文件里

image

//
// Tencent is pleased to support the open source community by making tRPC available.
//
// Copyright (C) 2023 THL A29 Limited, a Tencent company.
Copy link
Contributor

Choose a reason for hiding this comment

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

之前没注意,这里可以改成2024

visibility = [
"//visibility:public",
],
linkopts =["-lssl","-lcrypto"],
Copy link
Contributor

Choose a reason for hiding this comment

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

这两个链接选项之前讨论过,应该不需要吧?可以删掉

// possible hash functions, by using SIMD instructions, or by
// compromising on hash quality.

#include "City.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

头文件路径改成:trpc/naming/common/util/hash/City.h
保持和项目其他文件的一致性,.cpp文件名改成.cc

//
//

#include <openssl/md5.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

看到openssl只用到了md5,是否可以不用引入openssl库,只是把md5工具函数给抽出来,提高hash模块的独立性(要不是用hash算法的业务都需要引入ssl,太重了)

#include <string>
#include <unordered_map>
#include <vector>
#include "trpc/naming/common/util/hash/City.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

代码规范有点问题,21行和22行之间加个空格

#include <vector>
#include "trpc/naming/common/util/hash/City.h"
#include "trpc/naming/common/util/hash/MurmurHash3.h"
#include "trpc/naming/common/util/hash/hash_func.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

实现.h头文件定义接口的cc文件,include放在第一个(放在includec/c++头文件之前并以空格隔开)

return false;
}

std::string ModuloHashLoadBalance::GenerateKeysAsString(const SelectorInfo* info, std::vector<uint32_t> indexs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

indexs与配置相关,如果业务配了不合理的数值(比如-1/6),则会失败,Init里可以做一下这块检查,检查不通过返回失败

Copy link
Contributor Author

Choose a reason for hiding this comment

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

没有找到loadbalance配置和错误配置都返回false吗

Copy link
Contributor

Choose a reason for hiding this comment

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

错误配置返回false就好


mutable std::shared_mutex mutex_;

// std::unordered_map<std::string,
Copy link
Contributor

Choose a reason for hiding this comment

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

删除无用注释


InnerEndpointInfos old_info;

if (IsLoadBalanceInfoDiff(info, old_info)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IsLoadBalanceInfoDiff看接口名只会检查,建议不要在里面做old_info的赋值


result.result = info_iter->second;
// set hash to the next value
iter->second.hash = info_iter->first + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

这里没看懂,可以说明下为啥要+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

当前选择了第一个大于等于hash值的元素,然后我将hash值设置为选择元素的hash值+1,这样可以选到顺位的下一个元素

Copy link
Contributor

Choose a reason for hiding this comment

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

看懂了,你是用了做下一次hash环的查找。

Copy link
Contributor

Choose a reason for hiding this comment

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

但你的这种查找,不能让同一个客户端连续发2次调用,都发到同一个服务端节点上去吧

Copy link
Contributor

@helloopenworld helloopenworld left a comment

Choose a reason for hiding this comment

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

approve

@helloopenworld helloopenworld merged commit 5578377 into trpc-group:main Sep 18, 2024
1 check passed
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.

3 participants