-
Notifications
You must be signed in to change notification settings - Fork 439
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
issue=#1271 Add mock zk adapter for test #1272
Conversation
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.
我觉得在TabletNodeZkAdapterBase 这里增加OnKickMarkCreated()和OnLockChange() 这两个接口, 然后再实现一个支持zk的mock类是不是更好些?
@@ -132,6 +132,16 @@ class InsTabletNodeZkAdapter : public TabletNodeZkAdapterBase { | |||
galaxy::ins::sdk::InsSDK* ins_sdk_; | |||
}; | |||
|
|||
class MockInsTabletNodeZkAdapter : public InsTabletNodeZkAdapter { |
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.
建议把类的名字改成NotExistMockInsTabletNodeZkAdapter, 这样是不是更清晰些?
另外是不是也支持下mock zk, 以后即使用zk的话, 这段测试代码仍然可用?
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.
写成Mock主要是为了方便后面的扩展,比如去模拟nexus其它的异常之类
zk看了下,直接做成mock不太好做,主要是由于zk本身的回调以及一些虚函数之类的问题,导致代码不太好复用,只能重新实现
src/master/master_impl.cc
Outdated
@@ -169,6 +170,9 @@ bool MasterImpl::Init() { | |||
} else if (FLAGS_tera_ins_enabled) { | |||
LOG(INFO) << "ins mode" ; | |||
zk_adapter_.reset(new InsMasterZkAdapter(this, local_addr_)); | |||
} else if (FLAGS_tera_mock_ins_enabled) { |
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.
master不需要支持这个mock吧, 只在ts上支持?
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.
是的,这样做主要是为了不同flag的统一,不增加过多的flag,比如不可能ts用一个mock_ins,master用一个正常的ins
LGTM |
src/master/master_impl.cc
Outdated
@@ -166,9 +168,14 @@ MasterImpl::~MasterImpl() { | |||
bool MasterImpl::Init() { | |||
if (FLAGS_tera_zk_enabled) { | |||
zk_adapter_.reset(new MasterZkAdapter(this, local_addr_)); | |||
} else if (FLAGS_tera_mock_zk_enabled) { | |||
zk_adapter_.reset(new MockMasterZkAdapter(this, local_addr_)); | |||
} else if (FLAGS_tera_ins_enabled) { |
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.
调整一下优先级吧,只有zk_enabled = false, ins_enabled = false, 才看mock zk和mock ins;测试优先级应该比较低,防止误操作
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.
其实这块本来就有问题,按道理这几个都应该是互斥的,应该搞成只有一个flag
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.
优先级都统一的话,出错概率会降低
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.
不是出错概率啊,是本来就不该出错
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.
那为什么要引入出错的可能呢
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.
都升级版本了,怎么着也得改了吧,配置应该是和二进制配套的
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.
我指的是sdk端tera.flag的如何配套修改
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.
如果是老的sdk,不用改,新的sdk直接用
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.
总结起来,大致是两个问题:
1、如果改成字符串的话,导致sdk端升级时,必须确保客户端配套修改tera.flag;升级ts和master的话,必须配套升级tera.flag,不能单升级二进制;
2、如果不改字符串的话,应该确保sdk,master,ts优先级一致,配置容易维护;
ps:如果大家觉得有问题的话,就讨论一次吧
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.
2、如果不改字符串的话,应该确保sdk,master,ts优先级一致,配置容易维护;
ps:如果大家觉得有问题的话,就讨论一次吧
我也认同杰明的建议, 按照这个来吧
src/sdk/sdk_zk.cc
Outdated
} else if (FLAGS_tera_mock_ins_enabled) { | ||
return new sdk::MockInsClusterFinder(FLAGS_tera_ins_root_path, FLAGS_tera_ins_addr_list); | ||
} else if (FLAGS_tera_mock_zk_enabled) { | ||
return new sdk::MockZkClusterFinder(FLAGS_tera_zk_root_path, FLAGS_tera_zk_addr_list); | ||
} else if (!FLAGS_tera_zk_enabled) { |
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.
优先级同上
src/tabletnode/tabletnode_impl.cc
Outdated
} else if(FLAGS_tera_ins_enabled) { | ||
LOG(INFO) << "ins mode!"; | ||
zk_adapter_.reset(new InsTabletNodeZkAdapter(this, local_addr_)); | ||
} else if (FLAGS_tera_mock_ins_enabled) { | ||
LOG(INFO) << "mock ins mode!"; | ||
zk_adapter_.reset(new MockInsTabletNodeZkAdapter(this, local_addr_)); | ||
} else { |
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.
优先级同上
LGTM |
No description provided.