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

test: add msg and refresh for coverage test #3413

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/client/tablet_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,17 @@ bool TabletClient::UpdateTTL(uint32_t tid, uint32_t pid, const ::openmldb::type:
return false;
}

bool TabletClient::Refresh() {
::openmldb::api::RefreshRequest request;
::openmldb::api::GeneralResponse response;
bool ret = client_.SendRequest(&::openmldb::api::TabletServer_Stub::Refresh, &request, &response,
FLAGS_request_timeout_ms, FLAGS_request_max_retry);
if (!ret || response.code() != 0) {
return false;
}
return true;
}

bool TabletClient::Refresh(uint32_t tid) {
::openmldb::api::RefreshRequest request;
request.set_tid(tid);
Expand Down
1 change: 1 addition & 0 deletions src/client/tablet_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ class TabletClient : public Client {

bool DropProcedure(const std::string& db_name, const std::string& sp_name);

bool Refresh();
bool Refresh(uint32_t tid);

bool SubQuery(const ::openmldb::api::QueryRequest& request,
Expand Down
13 changes: 9 additions & 4 deletions src/cmd/sql_cmd_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2998,8 +2998,12 @@ TEST_P(DBSDKTest, LongWindowsCleanup) {
HandleSQL("create database test2;");
HandleSQL("use test2;");
HandleSQL(create_sql);
sleep(5);
// TODO if refresh is not good, sleep more
Copy link
Contributor

Choose a reason for hiding this comment

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

[cpplint] reported by reviewdog 🐶
Missing username in TODO; it should look like "// TODO(my_username): Stuff." [readability/todo] [2]

// sp_cache_->ProcedureExist in tablet get deployment here, but nameserver no deployment
// refresh won't effet sp_cache_ in tablet
sr->ExecuteSQL(deploy_sql, &status);
ASSERT_TRUE(status.IsOK());
ASSERT_TRUE(status.IsOK()) << status.ToString();
std::string msg;
std::string result_sql = "select * from __INTERNAL_DB.PRE_AGG_META_INFO;";
auto rs = sr->ExecuteSQL("", result_sql, &status);
Expand All @@ -3014,6 +3018,7 @@ TEST_P(DBSDKTest, LongWindowsCleanup) {
ASSERT_FALSE(cs->GetNsClient()->DropTable("test2", "trans", msg));
ASSERT_TRUE(cs->GetNsClient()->DropProcedure("test2", "demo1", msg)) << msg;
ASSERT_TRUE(cs->GetNsClient()->DropTable("test2", "trans", msg)) << msg;
ASSERT_TRUE(sr->RefreshCatalog()); // avoid cache in sdk
// helpful for debug
HandleSQL("show tables;");
HandleSQL("show deployments;");
Expand All @@ -3032,10 +3037,10 @@ TEST_P(DBSDKTest, CreateWithoutIndexCol) {
"c8 date, index(ts=c7));";
hybridse::sdk::Status status;
sr->ExecuteSQL(create_sql, &status);
ASSERT_TRUE(status.IsOK());
ASSERT_TRUE(status.IsOK()) << status.ToString();
std::string msg;
ASSERT_TRUE(cs->GetNsClient()->DropTable("test2", "trans", msg));
ASSERT_TRUE(cs->GetNsClient()->DropDatabase("test2", msg));
ASSERT_TRUE(cs->GetNsClient()->DropTable("test2", "trans", msg)) << msg;
ASSERT_TRUE(cs->GetNsClient()->DropDatabase("test2", msg)) << msg;
}

TEST_P(DBSDKTest, CreateIfNotExists) {
Expand Down
18 changes: 17 additions & 1 deletion src/nameserver/name_server_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8586,7 +8586,7 @@ bool NameServerImpl::AddIndexToTableInfo(const std::string& name, const std::str
endpoint_set.insert(meta.endpoint());
}
}
// locked on top
// locked on top TODO(hw):
for (const auto& tablet : tablets_) {
if (!tablet.second->Health()) {
continue;
Expand Down Expand Up @@ -9428,6 +9428,8 @@ void NameServerImpl::DropProcedure(RpcController* controller, const api::DropPro
db_sp_info_map_.erase(db_name);
}
NotifyTableChanged(::openmldb::type::NotifyType::kTable);
// Refresh on tablet to avoid meta inconsistent, notify may be slow TODO refresh works on procedure?
RefreshHealthTabletsUnlockWith([](const std::shared_ptr<TabletInfo>& tablet_info) { return true; });
}
response->set_code(::openmldb::base::ReturnCode::kOk);
response->set_msg("ok");
Expand Down Expand Up @@ -10511,5 +10513,19 @@ base::Status NameServerImpl::CreateDeployOP(const DeploySQLRequest& request, uin
return {};
}

template <typename T>
inline bool NameServerImpl::RefreshHealthTabletsUnlockWith(T pred) {
bool ret = true;
for (const auto& tablet : tablets_) {
if (!tablet.second->Health()) {
continue;
}
if (pred(tablet.second)) {
ret &= tablet.second->client_->Refresh();
}
}
return ret;
}

} // namespace nameserver
} // namespace openmldb
2 changes: 2 additions & 0 deletions src/nameserver/name_server_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,8 @@ class NameServerImpl : public NameServer {

bool IsExistDataBase(const std::string& db);

template<typename T> bool RefreshHealthTabletsUnlockWith(T pred);

private:
std::mutex mu_;
Tablets tablets_;
Expand Down
4 changes: 3 additions & 1 deletion src/tablet/sql_cluster_availability_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,10 @@ TEST_F(SqlClusterTest, RecoverProcedure) {
::openmldb::tablet::TabletImpl* tablet2 = new ::openmldb::tablet::TabletImpl();
StartTablet(&tb_server2, tablet2);
sleep(3);
// ensure tablet added in sdk
ASSERT_TRUE(router->RefreshCatalog());
rs = router->CallProcedure(db, sp_name, request_row, &status);
if (!rs) FAIL() << "call procedure failed";
if (!rs) FAIL() << "call procedure failed, " + status.ToString();
schema = rs->GetSchema();
ASSERT_EQ(schema->GetColumnCnt(), 3);
ASSERT_TRUE(rs->Next());
Expand Down
13 changes: 10 additions & 3 deletions src/tablet/tablet_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -553,8 +553,13 @@ void TabletImpl::Refresh(RpcController* controller, const ::openmldb::api::Refre
::openmldb::api::GeneralResponse* response, Closure* done) {
brpc::ClosureGuard done_guard(done);
if (IsClusterMode()) {
if (RefreshSingleTable(request->tid())) {
PDLOG(INFO, "refresh success. tid %u", request->tid());
if(request->has_tid()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[cpplint] reported by reviewdog 🐶
Missing space before ( in if( [whitespace/parens] [5]

if (RefreshSingleTable(request->tid())) {
PDLOG(INFO, "refresh success. tid %u", request->tid());
}
} else {
LOG(INFO) << "refresh table info by RefreshRequest without tid";
RefreshTableInfo();
}
}
}
Expand Down Expand Up @@ -5219,6 +5224,7 @@ void TabletImpl::CreateProcedure(RpcController* controller, const openmldb::api:
const std::string& db_name = sp_info.db_name();
const std::string& sp_name = sp_info.sp_name();
const std::string& sql = sp_info.sql();
LOG(INFO) << "create procedure rpc in " << endpoint_; // no get size func << " with sp cache " << sp_cache_->
Copy link
Contributor

Choose a reason for hiding this comment

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

[cpplint] reported by reviewdog 🐶
At least two spaces is best between code and comments [whitespace/comments] [2]

if (sp_cache_->ProcedureExist(db_name, sp_name)) {
response->set_code(::openmldb::base::ReturnCode::kProcedureAlreadyExists);
response->set_msg("store procedure already exists");
Expand Down Expand Up @@ -5283,7 +5289,7 @@ void TabletImpl::CreateProcedure(RpcController* controller, const openmldb::api:

response->set_code(::openmldb::base::ReturnCode::kOk);
response->set_msg("ok");
LOG(INFO) << "create procedure success! sp_name: " << sp_name << ", db: " << db_name << ", sql: " << sql;
LOG(INFO) << "create procedure success! sp_name: " << sp_name << ", db: " << db_name << ", sql: " << sql << " on " << endpoint_;
Copy link
Contributor

Choose a reason for hiding this comment

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

[cpplint] reported by reviewdog 🐶
Lines should be <= 120 characters long [whitespace/line_length] [2]

}

void TabletImpl::DropProcedure(RpcController* controller, const ::openmldb::api::DropProcedureRequest* request,
Expand Down Expand Up @@ -5311,6 +5317,7 @@ void TabletImpl::DropProcedure(RpcController* controller, const ::openmldb::api:
}
response->set_code(::openmldb::base::ReturnCode::kOk);
response->set_msg("ok");
LOG(INFO) << "drop succ in " << endpoint_;
PDLOG(INFO, "drop procedure success. db_name[%s] sp_name[%s]", db_name.c_str(), sp_name.c_str());
}

Expand Down