From 49a246141119036ab9fa6e5990daeefb2469b022 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Sat, 20 Mar 2021 22:16:47 +0100 Subject: [PATCH 1/2] Sink: add in memory sink, slightly refactor validation/testing. - Adds an in-memory sink. Comes in handy in tests, and could serve a future role if we want to converge the way results are handled in both distributed and non-distributed tests. - Slightly refactors validation & testing to make it easier to see what is specific to FileSinkImpl, making things a little more natural in case someone else wants to add a new sink type in the future. - Avoid MOCK_METHOD_N in the Sink mock as per recommendation. Signed-off-by: Otto van der Schaaf --- include/nighthawk/sink/sink.h | 2 +- source/sink/sink_impl.cc | 45 +++++++++++++++++++++++++++++++---- source/sink/sink_impl.h | 18 +++++++++++++- test/mocks/sink/mock_sink.h | 9 ++++--- test/sink/sink_test.cc | 24 +++++++------------ 5 files changed, 71 insertions(+), 27 deletions(-) diff --git a/include/nighthawk/sink/sink.h b/include/nighthawk/sink/sink.h index c35e48dcd..10eeb6ce2 100644 --- a/include/nighthawk/sink/sink.h +++ b/include/nighthawk/sink/sink.h @@ -30,7 +30,7 @@ class Sink { * @return absl::Status Indicates if the operation succeeded or not. */ virtual absl::Status - StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) const PURE; + StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) PURE; /** * Attempt to load a vector of ExecutionResponse instances associated to an execution id. diff --git a/source/sink/sink_impl.cc b/source/sink/sink_impl.cc index ad15df326..f83075c7d 100644 --- a/source/sink/sink_impl.cc +++ b/source/sink/sink_impl.cc @@ -36,12 +36,22 @@ absl::Status verifyCanBeUsedAsDirectoryName(absl::string_view s) { return absl::OkStatus(); } +absl::Status validateKey(absl::string_view key, const bool validate_as_directory_name) { + absl::Status status = + key.empty() ? absl::Status(absl::StatusCode::kInvalidArgument, "empty key is not allowed.") + : absl::OkStatus(); + if (status.ok() && validate_as_directory_name) { + status = verifyCanBeUsedAsDirectoryName(key); + } + return status; +} + } // namespace -absl::Status FileSinkImpl::StoreExecutionResultPiece( - const nighthawk::client::ExecutionResponse& response) const { +absl::Status +FileSinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) { const std::string& execution_id = response.execution_id(); - absl::Status status = verifyCanBeUsedAsDirectoryName(execution_id); + absl::Status status = validateKey(execution_id, true); if (!status.ok()) { return status; } @@ -74,11 +84,10 @@ absl::Status FileSinkImpl::StoreExecutionResultPiece( absl::StatusOr> FileSinkImpl::LoadExecutionResult(absl::string_view execution_id) const { - absl::Status status = verifyCanBeUsedAsDirectoryName(execution_id); + absl::Status status = validateKey(execution_id, true); if (!status.ok()) { return status; } - std::filesystem::path filesystem_directory_path("/tmp/nh/" + std::string(execution_id) + "/"); std::vector responses; std::error_code error_code; @@ -104,4 +113,30 @@ FileSinkImpl::LoadExecutionResult(absl::string_view execution_id) const { return responses; } +absl::Status +InMemorySinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) { + absl::Status status = validateKey(response.execution_id(), false); + if (!status.ok()) { + return status; + } + auto iterator = + map_.insert({response.execution_id(), std::vector()}) + .first; + iterator->second.push_back(response); + return absl::Status(); +} + +absl::StatusOr> +InMemorySinkImpl::LoadExecutionResult(absl::string_view execution_id) const { + absl::Status status = validateKey(execution_id, false); + if (!status.ok()) { + return status; + } + auto iterator = map_.find(execution_id); + if (iterator != map_.end()) { + return (*iterator).second; + } + return absl::Status(absl::StatusCode::kNotFound, "Not found"); +} + } // namespace Nighthawk diff --git a/source/sink/sink_impl.h b/source/sink/sink_impl.h index 7c3013b11..a9b87a3c0 100644 --- a/source/sink/sink_impl.h +++ b/source/sink/sink_impl.h @@ -2,6 +2,8 @@ #include "nighthawk/sink/sink.h" +#include "absl/container/flat_hash_map.h" + namespace Nighthawk { /** @@ -11,9 +13,23 @@ namespace Nighthawk { class FileSinkImpl : public Sink { public: absl::Status - StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) const override; + StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) override; + absl::StatusOr> + LoadExecutionResult(absl::string_view id) const override; +}; + +/** + * Memory based implementation of Sink. + */ +class InMemorySinkImpl : public Sink { +public: + absl::Status + StoreExecutionResultPiece(const nighthawk::client::ExecutionResponse& response) override; absl::StatusOr> LoadExecutionResult(absl::string_view id) const override; + +private: + absl::flat_hash_map> map_; }; } // namespace Nighthawk diff --git a/test/mocks/sink/mock_sink.h b/test/mocks/sink/mock_sink.h index 3d57d6f14..3315d23fb 100644 --- a/test/mocks/sink/mock_sink.h +++ b/test/mocks/sink/mock_sink.h @@ -9,11 +9,10 @@ namespace Nighthawk { class MockSink : public Sink { public: MockSink(); - MOCK_CONST_METHOD1(StoreExecutionResultPiece, - absl::Status(const nighthawk::client::ExecutionResponse&)); - MOCK_CONST_METHOD1( - LoadExecutionResult, - absl::StatusOr>(absl::string_view)); + MOCK_METHOD(absl::Status, StoreExecutionResultPiece, + (const nighthawk::client::ExecutionResponse&)); + MOCK_METHOD(absl::StatusOr>, + LoadExecutionResult, (absl::string_view), (const)); }; } // namespace Nighthawk diff --git a/test/sink/sink_test.cc b/test/sink/sink_test.cc index df2169743..edd8018c4 100644 --- a/test/sink/sink_test.cc +++ b/test/sink/sink_test.cc @@ -11,7 +11,7 @@ namespace Nighthawk { namespace { // Future sink implementations register here for testing top level generic sink behavior. -using SinkTypes = testing::Types; +using SinkTypes = testing::Types; template class TypedSinkTest : public testing::Test { public: @@ -55,7 +55,7 @@ TYPED_TEST(TypedSinkTest, EmptyKeyStoreFails) { const absl::Status status = sink.StoreExecutionResultPiece(result_to_store); ASSERT_FALSE(status.ok()); EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument); - EXPECT_EQ(status.message(), "'' is not a guid: bad string length."); + EXPECT_EQ(status.message(), "empty key is not allowed."); } TYPED_TEST(TypedSinkTest, EmptyKeyLoadFails) { @@ -63,8 +63,7 @@ TYPED_TEST(TypedSinkTest, EmptyKeyLoadFails) { const auto status_or_execution_responses = sink.LoadExecutionResult(""); ASSERT_EQ(status_or_execution_responses.ok(), false); EXPECT_EQ(status_or_execution_responses.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_EQ(status_or_execution_responses.status().message(), - "'' is not a guid: bad string length."); + EXPECT_EQ(status_or_execution_responses.status().message(), "empty key is not allowed."); } TYPED_TEST(TypedSinkTest, Append) { @@ -79,13 +78,8 @@ TYPED_TEST(TypedSinkTest, Append) { EXPECT_EQ(status_or_execution_responses.value().size(), 2); } -// As of today, we constrain execution id to a guid. This way the file sink implementation -// ensures that it can safely use it to create directories. In the future, other sinks may not -// have to worry about such things. In that case it makes sense to add a validation call -// to the sink interface to make this implementation specific, and make the tests below -// implementation specific too. -TYPED_TEST(TypedSinkTest, BadGuidShortString) { - TypeParam sink; +TEST(FileSinkTest, BadGuidShortString) { + FileSinkImpl sink; const auto status_or_execution_responses = sink.LoadExecutionResult("14e75b2a-3e31-4a62-9279-add1e54091f"); ASSERT_EQ(status_or_execution_responses.ok(), false); @@ -94,8 +88,8 @@ TYPED_TEST(TypedSinkTest, BadGuidShortString) { "'14e75b2a-3e31-4a62-9279-add1e54091f' is not a guid: bad string length."); } -TYPED_TEST(TypedSinkTest, BadGuidBadDashPlacement) { - TypeParam sink; +TEST(FileSinkTest, BadGuidBadDashPlacement) { + FileSinkImpl sink; const auto status_or_execution_responses = sink.LoadExecutionResult("14e75b2a3-e31-4a62-9279-add1e54091f9"); ASSERT_EQ(status_or_execution_responses.ok(), false); @@ -105,8 +99,8 @@ TYPED_TEST(TypedSinkTest, BadGuidBadDashPlacement) { "positions not met."); } -TYPED_TEST(TypedSinkTest, BadGuidInvalidCharacter) { - TypeParam sink; +TEST(FileSinkTest, BadGuidInvalidCharacter) { + FileSinkImpl sink; const auto status_or_execution_responses = sink.LoadExecutionResult("14e75b2a-3e31-4x62-9279-add1e54091f9"); ASSERT_EQ(status_or_execution_responses.ok(), false); From 135e803b211c9d12ab165b26eeff572699b8598d Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 23 Mar 2021 08:56:41 +0100 Subject: [PATCH 2/2] Review feedback Signed-off-by: Otto van der Schaaf --- source/sink/sink_impl.cc | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/source/sink/sink_impl.cc b/source/sink/sink_impl.cc index f83075c7d..f7e9715d6 100644 --- a/source/sink/sink_impl.cc +++ b/source/sink/sink_impl.cc @@ -16,27 +16,25 @@ absl::Status verifyCanBeUsedAsDirectoryName(absl::string_view s) { const std::string err_template = "'{}' is not a guid: {}"; if (s.size() != reference_value.size()) { - return absl::Status(absl::StatusCode::kInvalidArgument, - fmt::format(err_template, s, "bad string length.")); + return absl::InvalidArgumentError(fmt::format(err_template, s, "bad string length.")); } for (size_t i = 0; i < s.size(); i++) { if (reference_value[i] == '-') { if (s[i] != '-') { - return absl::Status( - absl::StatusCode::kInvalidArgument, + return absl::InvalidArgumentError( fmt::format(err_template, s, "expectations around '-' positions not met.")); } continue; } if (!std::isxdigit(s[i])) { - return absl::Status(absl::StatusCode::kInvalidArgument, - fmt::format(err_template, s, "unexpected character encountered.")); + return absl::InvalidArgumentError( + fmt::format(err_template, s, "unexpected character encountered.")); } } return absl::OkStatus(); } -absl::Status validateKey(absl::string_view key, const bool validate_as_directory_name) { +absl::Status validateKey(absl::string_view key, bool validate_as_directory_name) { absl::Status status = key.empty() ? absl::Status(absl::StatusCode::kInvalidArgument, "empty key is not allowed.") : absl::OkStatus(); @@ -59,7 +57,7 @@ FileSinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionRespon std::filesystem::create_directories("/tmp/nh/" + std::string(execution_id) + "/", error_code); // Note error_code will not be set if an existing directory existed. if (error_code.value()) { - return absl::Status(absl::StatusCode::kInternal, error_code.message()); + return absl::InternalError(error_code.message()); } // Write to a tmp file, and if that succeeds, we swap it atomically to the target path, // to make the completely written file visible to consumers of LoadExecutionResult. @@ -68,7 +66,7 @@ FileSinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionRespon { std::ofstream ofs(uid.data(), std::ios_base::out | std::ios_base::binary); if (!response.SerializeToOstream(&ofs)) { - return absl::Status(absl::StatusCode::kInternal, "Failure writing to temp file"); + return absl::InternalError("Failure writing to temp file"); } } std::filesystem::path filesystem_path(uid.data()); @@ -76,10 +74,10 @@ FileSinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionRespon "/tmp/nh/" + std::string(execution_id) + "/" + std::string(filesystem_path.filename()); std::filesystem::rename(uid.data(), new_name, error_code); if (error_code.value()) { - return absl::Status(absl::StatusCode::kInternal, error_code.message()); + return absl::InternalError(error_code.message()); } ENVOY_LOG_MISC(trace, "Stored '{}'.", new_name); - return absl::Status(); + return absl::OkStatus(); } absl::StatusOr> @@ -100,15 +98,14 @@ FileSinkImpl::LoadExecutionResult(absl::string_view execution_id) const { nighthawk::client::ExecutionResponse response; std::ifstream ifs(it.path(), std::ios_base::binary); if (!response.ParseFromIstream(&ifs)) { - return absl::Status(absl::StatusCode::kInternal, - fmt::format("Failed to parse ExecutionResponse '{}'.", it.path())); + return absl::InternalError(fmt::format("Failed to parse ExecutionResponse '{}'.", it.path())); } else { ENVOY_LOG_MISC(trace, "Loaded '{}'.", it.path()); } responses.push_back(response); } if (error_code.value()) { - return absl::Status(absl::StatusCode::kNotFound, error_code.message()); + return absl::NotFoundError(error_code.message()); } return responses; } @@ -123,7 +120,7 @@ InMemorySinkImpl::StoreExecutionResultPiece(const nighthawk::client::ExecutionRe map_.insert({response.execution_id(), std::vector()}) .first; iterator->second.push_back(response); - return absl::Status(); + return absl::OkStatus(); } absl::StatusOr> @@ -136,7 +133,7 @@ InMemorySinkImpl::LoadExecutionResult(absl::string_view execution_id) const { if (iterator != map_.end()) { return (*iterator).second; } - return absl::Status(absl::StatusCode::kNotFound, "Not found"); + return absl::NotFoundError(fmt::format("No results found for execution-id: '{}'", execution_id)); } } // namespace Nighthawk