From d7ed9434bd94b98d6055f78195f0e28b70381c01 Mon Sep 17 00:00:00 2001 From: PINS Working Group Date: Mon, 11 Oct 2021 17:02:50 -0400 Subject: [PATCH] Add batch ops and StatusCode for PINS / P4Runtime * Add batch set/delete() to ProducerStateTable * Add StatusCode enum and functions to convert between string and enum values. * Allow exists() to check for whitespace. This is only to allow whitespace when we check for existence. We can already create entries with whitespace. * Add SWSS return code SWSS_RC_UNIMPLEMENTED * Fix json error, refer to https://github.com/nlohmann/json/issues/590 Submission containing materials of a third party: Copyright Google LLC; Licensed under Apache 2.0 Co-authored-by: Akarsh Gupta Co-authored-by: Jay Hu Co-authored-by: Manali Kumar Co-authored-by: Robert J. Halstead Co-authored-by: Runming Wu Co-authored-by: Yilan Ji Signed-off-by: Don Newton don@opennetworking.org --- common/dbconnector.cpp | 5 -- common/json.hpp | 2 +- common/producerstatetable.cpp | 123 ++++++++++++++++++++++++++++++++ common/producerstatetable.h | 13 ++++ common/status_code_util.h | 70 ++++++++++++++++++ tests/Makefile.am | 1 + tests/status_code_util_test.cpp | 19 +++++ 7 files changed, 227 insertions(+), 6 deletions(-) create mode 100644 common/status_code_util.h create mode 100644 tests/status_code_util_test.cpp diff --git a/common/dbconnector.cpp b/common/dbconnector.cpp index 13f90df9..65f31b85 100644 --- a/common/dbconnector.cpp +++ b/common/dbconnector.cpp @@ -627,11 +627,6 @@ int64_t DBConnector::del(const string &key) bool DBConnector::exists(const string &key) { RedisCommand rexists; - if (key.find_first_of(" \t") != string::npos) - { - SWSS_LOG_ERROR("EXISTS failed, invalid space or tab in single key: %s", key.c_str()); - throw runtime_error("EXISTS failed, invalid space or tab in single key"); - } rexists.format("EXISTS %s", key.c_str()); RedisReply r(this, rexists, REDIS_REPLY_INTEGER); return r.getContext()->integer > 0; diff --git a/common/json.hpp b/common/json.hpp index b2316010..5152517a 100644 --- a/common/json.hpp +++ b/common/json.hpp @@ -5394,7 +5394,7 @@ class basic_json { assert(lhs.m_value.array != nullptr); assert(rhs.m_value.array != nullptr); - return *lhs.m_value.array < *rhs.m_value.array; + return (*lhs.m_value.array) < *rhs.m_value.array; } case value_t::object: { diff --git a/common/producerstatetable.cpp b/common/producerstatetable.cpp index 1c23930f..d3b212c4 100644 --- a/common/producerstatetable.cpp +++ b/common/producerstatetable.cpp @@ -48,6 +48,35 @@ ProducerStateTable::ProducerStateTable(RedisPipeline *pipeline, const string &ta "end\n"; m_shaDel = m_pipe->loadRedisScript(luaDel); + string luaBatchedSet = + "local added = 0\n" + "local idx = 2\n" + "for i = 0, #KEYS - 4 do\n" + " added = added + redis.call('SADD', KEYS[2], KEYS[4 + i])\n" + " for j = 0, tonumber(ARGV[idx]) - 1 do\n" + " local attr = ARGV[idx + j * 2 + 1]\n" + " local val = ARGV[idx + j * 2 + 2]\n" + " redis.call('HSET', KEYS[3] .. KEYS[4 + i], attr, val)\n" + " end\n" + " idx = idx + tonumber(ARGV[idx]) * 2 + 1\n" + "end\n" + "if added > 0 then \n" + " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" + "end\n"; + m_shaBatchedSet = m_pipe->loadRedisScript(luaBatchedSet); + + string luaBatchedDel = + "local added = 0\n" + "for i = 0, #KEYS - 5 do\n" + " added = added + redis.call('SADD', KEYS[2], KEYS[5 + i])\n" + " redis.call('SADD', KEYS[3], KEYS[5 + i])\n" + " redis.call('DEL', KEYS[4] .. KEYS[5 + i])\n" + "end\n" + "if added > 0 then \n" + " redis.call('PUBLISH', KEYS[1], ARGV[1])\n" + "end\n"; + m_shaBatchedDel = m_pipe->loadRedisScript(luaBatchedDel); + string luaClear = "redis.call('DEL', KEYS[1])\n" "local keys = redis.call('KEYS', KEYS[2] .. '*')\n" @@ -156,6 +185,100 @@ void ProducerStateTable::del(const string &key, const string &op /*= DEL_COMMAND } } +void ProducerStateTable::set(const std::vector& values) +{ + if (m_tempViewActive) + { + // Write to temp view instead of DB + for (const auto &value : values) + { + const std::string &key = kfvKey(value); + for (const auto &iv : kfvFieldsValues(value)) + { + m_tempViewState[key][fvField(iv)] = fvValue(iv); + } + } + return; + } + + // Assembly redis command args into a string vector + vector args; + args.emplace_back("EVALSHA"); + args.emplace_back(m_shaBatchedSet); + args.emplace_back(to_string(values.size() + 3)); + args.emplace_back(getChannelName()); + args.emplace_back(getKeySetName()); + args.emplace_back(getStateHashPrefix() + getTableName() + getTableNameSeparator()); + for (const auto &value : values) + { + args.emplace_back(kfvKey(value)); + } + args.emplace_back("G"); + for (const auto &value : values) + { + args.emplace_back(to_string(kfvFieldsValues(value).size())); + for (const auto &iv : kfvFieldsValues(value)) + { + args.emplace_back(fvField(iv)); + args.emplace_back(fvValue(iv)); + } + } + + // Transform data structure + vector args1; + transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); + + // Invoke redis command + RedisCommand command; + command.formatArgv((int)args1.size(), &args1[0], NULL); + m_pipe->push(command, REDIS_REPLY_NIL); + if (!m_buffered) + { + m_pipe->flush(); + } +} + +void ProducerStateTable::del(const std::vector& keys) +{ + if (m_tempViewActive) + { + // Write to temp view instead of DB + for (const auto &key : keys) + { + m_tempViewState.erase(key); + } + return; + } + + // Assembly redis command args into a string vector + vector args; + args.emplace_back("EVALSHA"); + args.emplace_back(m_shaBatchedDel); + args.emplace_back(to_string(keys.size() + 4)); + args.emplace_back(getChannelName()); + args.emplace_back(getKeySetName()); + args.emplace_back(getDelKeySetName()); + args.emplace_back(getStateHashPrefix() + getTableName() + getTableNameSeparator()); + for (const auto &key : keys) + { + args.emplace_back(key); + } + args.emplace_back("G"); + + // Transform data structure + vector args1; + transform(args.begin(), args.end(), back_inserter(args1), [](const string &s) { return s.c_str(); } ); + + // Invoke redis command + RedisCommand command; + command.formatArgv((int)args1.size(), &args1[0], NULL); + m_pipe->push(command, REDIS_REPLY_NIL); + if (!m_buffered) + { + m_pipe->flush(); + } +} + void ProducerStateTable::flush() { m_pipe->flush(); diff --git a/common/producerstatetable.h b/common/producerstatetable.h index 7042749e..99559502 100644 --- a/common/producerstatetable.h +++ b/common/producerstatetable.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include "table.h" #include "redispipeline.h" @@ -35,6 +36,16 @@ class ProducerStateTable : public TableBase, public TableName_KeySet %} #endif + // Batched version of set() and del(). + // The batched methods don't include (or use) op and prefix. They are + // written for specific use case only. The consumer logic (or batch size) + // might need to change if the producer does batched operations. + + // In set(), the op is ignored. + void set(const std::vector& values); + + void del(const std::vector& keys); + void flush(); int64_t count(); @@ -51,6 +62,8 @@ class ProducerStateTable : public TableBase, public TableName_KeySet RedisPipeline *m_pipe; std::string m_shaSet; std::string m_shaDel; + std::string m_shaBatchedSet; + std::string m_shaBatchedDel; std::string m_shaClear; std::string m_shaApplyView; TableDump m_tempViewState; diff --git a/common/status_code_util.h b/common/status_code_util.h new file mode 100644 index 00000000..29ac16ea --- /dev/null +++ b/common/status_code_util.h @@ -0,0 +1,70 @@ +#pragma once + +#include +#include + +namespace swss { + +enum class StatusCode { + SWSS_RC_SUCCESS, + SWSS_RC_INVALID_PARAM, + SWSS_RC_DEADLINE_EXCEEDED, + SWSS_RC_UNAVAIL, + SWSS_RC_NOT_FOUND, + SWSS_RC_NO_MEMORY, + SWSS_RC_EXISTS, + SWSS_RC_PERMISSION_DENIED, + SWSS_RC_FULL, + SWSS_RC_IN_USE, + SWSS_RC_INTERNAL, + SWSS_RC_UNIMPLEMENTED, + SWSS_RC_UNKNOWN, +}; + +static std::map statusCodeMapping = { + {StatusCode::SWSS_RC_SUCCESS, "SWSS_RC_SUCCESS"}, + {StatusCode::SWSS_RC_INVALID_PARAM, "SWSS_RC_INVALID_PARAM"}, + {StatusCode::SWSS_RC_DEADLINE_EXCEEDED, "SWSS_RC_DEADLINE_EXCEEDED"}, + {StatusCode::SWSS_RC_UNAVAIL, "SWSS_RC_UNAVAIL"}, + {StatusCode::SWSS_RC_NOT_FOUND, "SWSS_RC_NOT_FOUND"}, + {StatusCode::SWSS_RC_NO_MEMORY, "SWSS_RC_NO_MEMORY"}, + {StatusCode::SWSS_RC_EXISTS, "SWSS_RC_EXISTS"}, + {StatusCode::SWSS_RC_PERMISSION_DENIED, "SWSS_RC_PERMISSION_DENIED"}, + {StatusCode::SWSS_RC_FULL, "SWSS_RC_FULL"}, + {StatusCode::SWSS_RC_IN_USE, "SWSS_RC_IN_USE"}, + {StatusCode::SWSS_RC_INTERNAL, "SWSS_RC_INTERNAL"}, + {StatusCode::SWSS_RC_UNIMPLEMENTED, "SWSS_RC_UNIMPLEMENTED"}, + {StatusCode::SWSS_RC_UNKNOWN, "SWSS_RC_UNKNOWN"}, +}; + +static std::map StatusCodeLookup = { + {"SWSS_RC_SUCCESS", StatusCode::SWSS_RC_SUCCESS}, + {"SWSS_RC_INVALID_PARAM", StatusCode::SWSS_RC_INVALID_PARAM}, + {"SWSS_RC_DEADLINE_EXCEEDED", StatusCode::SWSS_RC_DEADLINE_EXCEEDED}, + {"SWSS_RC_UNAVAIL", StatusCode::SWSS_RC_UNAVAIL}, + {"SWSS_RC_NOT_FOUND", StatusCode::SWSS_RC_NOT_FOUND}, + {"SWSS_RC_NO_MEMORY", StatusCode::SWSS_RC_NO_MEMORY}, + {"SWSS_RC_EXISTS", StatusCode::SWSS_RC_EXISTS}, + {"SWSS_RC_PERMISSION_DENIED", StatusCode::SWSS_RC_PERMISSION_DENIED}, + {"SWSS_RC_FULL", StatusCode::SWSS_RC_FULL}, + {"SWSS_RC_IN_USE", StatusCode::SWSS_RC_IN_USE}, + {"SWSS_RC_INTERNAL", StatusCode::SWSS_RC_INTERNAL}, + {"SWSS_RC_UNIMPLEMENTED", StatusCode::SWSS_RC_UNIMPLEMENTED}, + {"SWSS_RC_UNKNOWN", StatusCode::SWSS_RC_UNKNOWN}, +}; + +inline std::string statusCodeToStr(const StatusCode& status) { + if (statusCodeMapping.find(status) == statusCodeMapping.end()) { + return "SWSS_RC_UNKNOWN"; + } + return statusCodeMapping.at(status); +} + +inline StatusCode strToStatusCode(const std::string& status) { + if (StatusCodeLookup.find(status) == StatusCodeLookup.end()) { + return StatusCode::SWSS_RC_UNKNOWN; + } + return StatusCodeLookup.at(status); +} + +} // namespace swss \ No newline at end of file diff --git a/tests/Makefile.am b/tests/Makefile.am index 2067dd19..b1a76510 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -33,6 +33,7 @@ tests_SOURCES = redis_ut.cpp \ stringutility_ut.cpp \ redisutility_ut.cpp \ boolean_ut.cpp \ + status_code_util_test.cpp \ main.cpp tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) diff --git a/tests/status_code_util_test.cpp b/tests/status_code_util_test.cpp new file mode 100644 index 00000000..394a6aec --- /dev/null +++ b/tests/status_code_util_test.cpp @@ -0,0 +1,19 @@ +#include "common/status_code_util.h" + +#include + +namespace { + +using swss::StatusCode; + +TEST(StatusCodeUtilTest, StatusCodeUtilTest) { + for (int i = static_cast(StatusCode::SWSS_RC_SUCCESS); + i <= static_cast(StatusCode::SWSS_RC_UNKNOWN); ++i) { + StatusCode original = static_cast(i); + StatusCode final = swss::strToStatusCode(statusCodeToStr(original)); + EXPECT_EQ(original, final); + } + EXPECT_EQ(StatusCode::SWSS_RC_UNKNOWN, swss::strToStatusCode("invalid")); +} + +} // namespace \ No newline at end of file