Skip to content

Commit

Permalink
Merge 'api: task_manager: Return proper response status code' from Al…
Browse files Browse the repository at this point in the history 
…eksandra Martyniuk

Return 400 Bad Request instead of 500 Internal Server Error
when user requests task or module which does not exist through
task manager and task manager test apis.

Closes #14166

* github.com:scylladb/scylladb:
  test: add test checking response status when requested module does not exist
  api: fix indentation
  api: throw bad_param_exception when requested task/module does not exists
  • Loading branch information
@denesb
denesb committed Jun 8, 2023
2 parents 4f4d3f9 + 2e54a32 commit b4c21cf
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 44 deletions.
88 changes: 58 additions & 30 deletions api/task_manager.cc
View file
Expand Up @@ -119,7 +119,12 @@ void set_task_manager(http_context& ctx, routes& r, db::config& cfg) {
auto internal = tasks::is_internal{req_param<bool>(*req, "internal", false)};
std::vector<chunked_stats> res = co_await ctx.tm.map([&req, internal] (tasks::task_manager& tm) {
chunked_stats local_res;
auto module = tm.find_module(req->param["module"]);
tasks::task_manager::module_ptr module;
try {
module = tm.find_module(req->param["module"]);
} catch (...) {
throw bad_param_exception(fmt::format("{}", std::current_exception()));
}
const auto& filtered_tasks = module->get_tasks() | boost::adaptors::filtered([&params = req->query_parameters, internal] (const auto& task) {
return (internal || !task.second->is_internal()) && filter_tasks(task.second, params);
});
Expand Down Expand Up @@ -150,37 +155,51 @@ void set_task_manager(http_context& ctx, routes& r, db::config& cfg) {

tm::get_task_status.set(r, [&ctx] (std::unique_ptr<http::request> req) -> future<json::json_return_type> {
auto id = tasks::task_id{utils::UUID{req->param["task_id"]}};
auto task = co_await tasks::task_manager::invoke_on_task(ctx.tm, id, std::function([] (tasks::task_manager::task_ptr task) -> future<tasks::task_manager::foreign_task_ptr> {
auto state = task->get_status().state;
if (state == tasks::task_manager::task_state::done || state == tasks::task_manager::task_state::failed) {
task->unregister_task();
}
co_return std::move(task);
}));
tasks::task_manager::foreign_task_ptr task;
try {
task = co_await tasks::task_manager::invoke_on_task(ctx.tm, id, std::function([] (tasks::task_manager::task_ptr task) -> future<tasks::task_manager::foreign_task_ptr> {
auto state = task->get_status().state;
if (state == tasks::task_manager::task_state::done || state == tasks::task_manager::task_state::failed) {
task->unregister_task();
}
co_return std::move(task);
}));
} catch (tasks::task_manager::task_not_found& e) {
throw bad_param_exception(e.what());
}
auto s = co_await retrieve_status(task);
co_return make_status(s);
});

tm::abort_task.set(r, [&ctx] (std::unique_ptr<http::request> req) -> future<json::json_return_type> {
auto id = tasks::task_id{utils::UUID{req->param["task_id"]}};
co_await tasks::task_manager::invoke_on_task(ctx.tm, id, [] (tasks::task_manager::task_ptr task) -> future<> {
if (!task->is_abortable()) {
co_await coroutine::return_exception(std::runtime_error("Requested task cannot be aborted"));
}
co_await task->abort();
});
try {
co_await tasks::task_manager::invoke_on_task(ctx.tm, id, [] (tasks::task_manager::task_ptr task) -> future<> {
if (!task->is_abortable()) {
co_await coroutine::return_exception(std::runtime_error("Requested task cannot be aborted"));
}
co_await task->abort();
});
} catch (tasks::task_manager::task_not_found& e) {
throw bad_param_exception(e.what());
}
co_return json_void();
});

tm::wait_task.set(r, [&ctx] (std::unique_ptr<http::request> req) -> future<json::json_return_type> {
auto id = tasks::task_id{utils::UUID{req->param["task_id"]}};
auto task = co_await tasks::task_manager::invoke_on_task(ctx.tm, id, std::function([] (tasks::task_manager::task_ptr task) {
return task->done().then_wrapped([task] (auto f) {
task->unregister_task();
f.get();
return make_foreign(task);
});
}));
tasks::task_manager::foreign_task_ptr task;
try {
task = co_await tasks::task_manager::invoke_on_task(ctx.tm, id, std::function([] (tasks::task_manager::task_ptr task) {
return task->done().then_wrapped([task] (auto f) {
task->unregister_task();
f.get();
return make_foreign(task);
});
}));
} catch (tasks::task_manager::task_not_found& e) {
throw bad_param_exception(e.what());
}
auto s = co_await retrieve_status(task);
co_return make_status(s);
});
Expand All @@ -191,14 +210,19 @@ void set_task_manager(http_context& ctx, routes& r, db::config& cfg) {
std::queue<tasks::task_manager::foreign_task_ptr> q;
utils::chunked_vector<full_task_status> res;

// Get requested task.
auto task = co_await tasks::task_manager::invoke_on_task(_ctx.tm, id, std::function([] (tasks::task_manager::task_ptr task) -> future<tasks::task_manager::foreign_task_ptr> {
auto state = task->get_status().state;
if (state == tasks::task_manager::task_state::done || state == tasks::task_manager::task_state::failed) {
task->unregister_task();
}
co_return task;
}));
tasks::task_manager::foreign_task_ptr task;
try {
// Get requested task.
task = co_await tasks::task_manager::invoke_on_task(_ctx.tm, id, std::function([] (tasks::task_manager::task_ptr task) -> future<tasks::task_manager::foreign_task_ptr> {
auto state = task->get_status().state;
if (state == tasks::task_manager::task_state::done || state == tasks::task_manager::task_state::failed) {
task->unregister_task();
}
co_return task;
}));
} catch (tasks::task_manager::task_not_found& e) {
throw bad_param_exception(e.what());
}

// Push children's statuses in BFS order.
q.push(co_await task.copy()); // Task cannot be moved since we need it to be alive during whole loop execution.
Expand Down Expand Up @@ -228,7 +252,11 @@ void set_task_manager(http_context& ctx, routes& r, db::config& cfg) {

tm::get_and_update_ttl.set(r, [&cfg] (std::unique_ptr<http::request> req) -> future<json::json_return_type> {
uint32_t ttl = cfg.task_ttl_seconds();
co_await cfg.task_ttl_seconds.set_value_on_all_shards(req->query_parameters["ttl"], utils::config_file::config_source::API);
try {
co_await cfg.task_ttl_seconds.set_value_on_all_shards(req->query_parameters["ttl"], utils::config_file::config_source::API);
} catch (...) {
throw bad_param_exception(fmt::format("{}", std::current_exception()));
}
co_return json::json_return_type(ttl);
});
}
Expand Down
34 changes: 21 additions & 13 deletions api/task_manager_test.cc
View file
Expand Up @@ -71,10 +71,14 @@ void set_task_manager_test(http_context& ctx, routes& r) {

tmt::unregister_test_task.set(r, [&ctx] (std::unique_ptr<http::request> req) -> future<json::json_return_type> {
auto id = tasks::task_id{utils::UUID{req->query_parameters["task_id"]}};
co_await tasks::task_manager::invoke_on_task(ctx.tm, id, [] (tasks::task_manager::task_ptr task) -> future<> {
tasks::test_task test_task{task};
co_await test_task.unregister_task();
});
try {
co_await tasks::task_manager::invoke_on_task(ctx.tm, id, [] (tasks::task_manager::task_ptr task) -> future<> {
tasks::test_task test_task{task};
co_await test_task.unregister_task();
});
} catch (tasks::task_manager::task_not_found& e) {
throw bad_param_exception(e.what());
}
co_return json_void();
});

Expand All @@ -84,15 +88,19 @@ void set_task_manager_test(http_context& ctx, routes& r) {
bool fail = it != req->query_parameters.end();
std::string error = fail ? it->second : "";

co_await tasks::task_manager::invoke_on_task(ctx.tm, id, [fail, error = std::move(error)] (tasks::task_manager::task_ptr task) {
tasks::test_task test_task{task};
if (fail) {
test_task.finish_failed(std::make_exception_ptr(std::runtime_error(error)));
} else {
test_task.finish();
}
return make_ready_future<>();
});
try {
co_await tasks::task_manager::invoke_on_task(ctx.tm, id, [fail, error = std::move(error)] (tasks::task_manager::task_ptr task) {
tasks::test_task test_task{task};
if (fail) {
test_task.finish_failed(std::make_exception_ptr(std::runtime_error(error)));
} else {
test_task.finish();
}
return make_ready_future<>();
});
} catch (tasks::task_manager::task_not_found& e) {
throw bad_param_exception(e.what());
}
co_return json_void();
});
}
Expand Down
2 changes: 1 addition & 1 deletion test/rest_api/task_manager_utils.py
View file
Expand Up @@ -56,7 +56,7 @@ def check_status_correctness(status, expected_status):

def assert_task_does_not_exist(rest_api, task_id):
resp = rest_api.send("GET", f"task_manager/task_status/{task_id}")
assert resp.status_code == requests.codes.internal_server_error, f"Task {task_id} is kept in memory"
assert resp.status_code == requests.codes.bad_request, f"Task {task_id} is kept in memory"

def check_child_parent_relationship(rest_api, parent, tree_depth, depth=0):
assert parent["children_ids"], "Child tasks were not created"
Expand Down
5 changes: 5 additions & 0 deletions test/rest_api/test_task_manager.py
View file
Expand Up @@ -158,3 +158,8 @@ def test_task_manager_recursive_status(rest_api):
check_field_correctness("id", tasks[1], { "id" : f"{task1}" })
check_field_correctness("id", tasks[2], { "id" : f"{task3}" })
check_field_correctness("id", tasks[3], { "id" : f"{task2}" })

def test_module_not_exists(rest_api):
module_name = "module_that_does_not_exist"
resp = rest_api.send("GET", f"task_manager/list_module_tasks/{module_name}", )
assert resp.status_code == requests.codes.bad_request, f"Invalid response status code: {resp.status_code}"

0 comments on commit b4c21cf

Please sign in to comment.