acelyc111 commented on code in PR #2172:
URL:
https://github.com/apache/incubator-pegasus/pull/2172#discussion_r1900970310
##########
src/client/replication_ddl_client.cpp:
##########
@@ -1477,17 +1396,22 @@ void replication_ddl_client::end_meta_request(const
rpc_response_task_ptr &callb
dsn::error_code replication_ddl_client::get_app_envs(const std::string
&app_name,
std::map<std::string,
std::string> &envs)
{
+ // Just match the table with the provided name exactly since we want to
get the envs from
+ // a specific table.
std::vector<::dsn::app_info> apps;
- auto r = list_apps(dsn::app_status::AS_AVAILABLE, apps);
- if (r != dsn::ERR_OK) {
- return r;
+ const auto &result = list_apps(
Review Comment:
There are many code snippet like this, we can wrap it by a macro in next
patch to simplify the code.
```
auto a = xxx;
if (!a) {
return a.code();
}
```
##########
src/client/replication_ddl_client.cpp:
##########
@@ -1477,17 +1396,22 @@ void replication_ddl_client::end_meta_request(const
rpc_response_task_ptr &callb
dsn::error_code replication_ddl_client::get_app_envs(const std::string
&app_name,
std::map<std::string,
std::string> &envs)
{
+ // Just match the table with the provided name exactly since we want to
get the envs from
+ // a specific table.
std::vector<::dsn::app_info> apps;
- auto r = list_apps(dsn::app_status::AS_AVAILABLE, apps);
- if (r != dsn::ERR_OK) {
- return r;
+ const auto &result = list_apps(
+ dsn::app_status::AS_AVAILABLE, app_name,
utils::pattern_match_type::PMT_MATCH_EXACT, apps);
+ if (!result) {
+ return result.code();
}
- for (auto &app : apps) {
- if (app.app_name == app_name) {
- envs = app.envs;
- return dsn::ERR_OK;
+ for (const auto &app : apps) {
+ if (app.app_name != app_name) {
Review Comment:
While we match the app by name exactly by `PMT_MATCH_EXACT`, how can an
app's name doesn't equal to `app_name`?
##########
src/client/replication_ddl_client.h:
##########
@@ -95,22 +95,59 @@ class replication_ddl_client
error_with<configuration_rename_app_response> rename_app(const std::string
&old_app_name,
const std::string
&new_app_name);
- dsn::error_code list_apps(const dsn::app_status::type status,
- bool show_all,
- bool detailed,
- bool json,
- const std::string &file_name);
-
- dsn::error_code list_apps(const dsn::app_status::type status,
- std::vector<::dsn::app_info> &apps);
-
- dsn::error_code list_nodes(const dsn::replication::node_status::type
status,
- bool detailed,
- const std::string &file_name,
- bool resolve_ip = false);
-
+ // Choose tables and list them to a file, with path specified as
`output_file`. Once
+ // `output_file` is empty, tables would be listed to stdout.
+ //
+ // Choose tables according to following parameters:
+ // `show_all`: whether to show all tables, not only the available, but
also the ones in
+ // other status, e.g. the dropped tables.
+ // `detailed`: whether to show healthy/unhealthy details.
+ // `json`: whether to output as json format.
+ // `status`: the status of the tables chosen to be listed.
`app_status::AS_INVALID` means
+ // no restriction.
+ // `app_name_pattern`: the name pattern of the tables chosen to be listed.
+ // `match_type`: the type in which the name pattern would be matched.
+ error_s list_apps(bool show_all,
+ bool detailed,
+ bool json,
+ const std::string &output_file,
+ dsn::app_status::type status,
+ const std::string &app_name_pattern,
+ utils::pattern_match_type::type match_type);
+
+ // The same as the above, except that there's no restriction on table
name; in other
+ // words, the match type is `PMT_MATCH_ALL`.
+ error_s list_apps(bool show_all,
+ bool detailed,
+ bool json,
+ const std::string &output_file,
+ dsn::app_status::type status);
+
+ // Create and send request to meta server to get the tables chosen to be
listed according
+ // to the following parameters:
+ // `status`: the status of the tables chosen to be listed.
`app_status::AS_INVALID` means
+ // no restriction.
+ // `app_name_pattern`: the name pattern of the tables chosen to be listed.
+ // `match_type`: the type in which the name pattern would be matched.
Review Comment:
Do the 3 parameters have any difference with the ones of the above
functions? If not, they can be omitted to reduce confusion.
##########
src/client/replication_ddl_client.h:
##########
@@ -312,28 +354,52 @@ class replication_ddl_client
return task;
}
+ // The same as the above, except that the thread hash for the RPC response
task is set to 0.
+ template <typename TRequest>
+ rpc_response_task_ptr request_meta(const dsn::task_code &code,
+ std::shared_ptr<TRequest> &req,
+ int timeout_milliseconds)
+ {
+ return request_meta(code, req, timeout_milliseconds, 0);
+ }
+
+ // The same as the above, except that the timeout for the RPC request is
set to 0.
Review Comment:
Add code annotation to explain what does `0` mean to the function with
`timeout_milliseconds` parameter. (never timeout?)
##########
src/meta/meta_http_service.cpp:
##########
@@ -391,15 +400,14 @@ void meta_http_service::list_node_handler(const
http_request &req, http_response
for (const auto &node : _service->_dead_set) {
tmp_map.emplace(node, list_nodes_helper(node.to_string(), "UNALIVE"));
}
- int alive_node_count = (_service->_alive_set).size();
- int unalive_node_count = (_service->_dead_set).size();
+
+ size_t alive_node_count = (_service->_alive_set).size();
+ size_t unalive_node_count = (_service->_dead_set).size();
Review Comment:
```suggestion
size_t alive_node_count = _service->_alive_set.size();
size_t unalive_node_count = _service->_dead_set.size();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
