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

[sairedis/syncd] Implement bulk get support (#1509) #32

Open
wants to merge 1 commit into
base: 202412
Choose a base branch
from

Conversation

r12f
Copy link
Contributor

@r12f r12f commented Feb 28, 2025

DEPENDS ON sonic-net/sonic-swss-common#966.

Implement bulk get support for SAIRedis, Syncd, SaiPlayer and VSlib.

Copy link
Contributor

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

TestRedisRemoteSaiInterface.cpp needs to be removed from lib/Makefile. Else we will have compilation failure. Can we please remove?

@r12f
Copy link
Contributor Author

r12f commented Mar 1, 2025

I cherry picked Steven's commit. Maybe I can create a new pick from the source.

@dgsudharsan
Copy link
Contributor

@stepanblyschak We need this diff

git diff unittest/lib/Makefile.am
diff --git a/unittest/lib/Makefile.am b/unittest/lib/Makefile.am
index 62e3a187..ca95f647 100644
--- a/unittest/lib/Makefile.am
+++ b/unittest/lib/Makefile.am
@@ -24,7 +24,6 @@ tests_SOURCES = \
                                TestRecorder.cpp \
                                TestRedisChannel.cpp \
                                TestClientSai.cpp \
-                               TestRedisRemoteSaiInterface.cpp \
                                TestServerSai.cpp \
                                TestSai.cpp

@r12f r12f force-pushed the cherry-pick-1509 branch from f465fe5 to 281f58e Compare March 1, 2025 06:16
DEPENDS ON sonic-net/sonic-swss-common#966.

Implement bulk get support for SAIRedis, Syncd, SaiPlayer and VSlib.
@r12f r12f force-pushed the cherry-pick-1509 branch from 281f58e to 995e71f Compare March 1, 2025 06:37

for (uint32_t idx = 0; idx < object_count; idx++)
{
if (object_statuses[idx] == SAI_STATUS_SUCCESS)

Check notice

Code scanning / CodeQL

FIXME comment Note

FIXME comment: This macro returns on failure.
Comment on lines 2266 to 2359
switch (api)
{
case SAI_COMMON_API_BULK_CREATE:

{
sai_object_id_t switch_id = m_sai->switchIdQuery(local_ids[0]);
std::vector<sai_object_id_t> ids(object_count);

for (uint32_t it = 0; it < object_count; it++)
{
if (m_sai->switchIdQuery(local_ids[it]) != switch_id ||
switch_id == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_THROW("invalid switch_id translated from VID %s",
sai_serialize_object_id(local_ids[it]).c_str());
}
}

std::vector<uint32_t> attr_counts(object_count);

std::vector<const sai_attribute_t*> attr_lists(object_count);

for (uint32_t idx = 0; idx < object_count; idx++)
{
attr_counts[idx] = attributes[idx]->get_attr_count();
attr_lists[idx] = attributes[idx]->get_attr_list();
}

switch_id = translate_local_to_redis(switch_id);

status = m_sai->bulkCreate(object_type,
switch_id,
object_count,
attr_counts.data(),
attr_lists.data(),
mode,
ids.data(),
statuses.data());

if (status == SAI_STATUS_SUCCESS)
{
for (uint32_t it = 0; it < object_count; it++)
{
match_redis_with_rec(ids[it], local_ids[it]);

SWSS_LOG_INFO("saved VID %s to RID %s",
sai_serialize_object_id(local_ids[it]).c_str(),
sai_serialize_object_id(ids[it]).c_str());
}
}

return status;
}
break;

case SAI_COMMON_API_BULK_REMOVE:

{
std::vector<sai_object_id_t> ids(object_count);

for (uint32_t it = 0; it < object_count; it++)
{
ids[it] = translate_local_to_redis(local_ids[it]);
}

status = m_sai->bulkRemove(object_type, object_count, ids.data(), mode, statuses.data());
}
break;

case SAI_COMMON_API_BULK_SET:

{
std::vector<sai_object_id_t> ids(object_count);

for (uint32_t it = 0; it < object_count; it++)
{
ids[it] = translate_local_to_redis(local_ids[it]);
}

std::vector<sai_attribute_t> attr_list;

// route can have multiple attributes, so we need to handle them all
for (const auto &alist: attributes)
{
attr_list.push_back(alist->get_attr_list()[0]);
}

status = m_sai->bulkSet(object_type, object_count, ids.data(), attr_list.data(), mode, statuses.data());
}
break;

case SAI_COMMON_API_BULK_GET:

{

Check notice

Code scanning / CodeQL

Long switch case Note

Switch has at least one case that is too long:
SAI_COMMON_API_BULK_CREATE (52 lines)
.
Comment on lines +671 to +680
switch (object_type)
{
case SAI_OBJECT_TYPE_PORT:
ptr = m_apis.port_api->get_ports_attribute;
break;

SWSS_LOG_ERROR("not implemented, FIXME");
default:
SWSS_LOG_ERROR("not implemented %s, FIXME", sai_serialize_object_type(object_type).c_str());
return SAI_STATUS_NOT_IMPLEMENTED;
}

Check notice

Code scanning / CodeQL

No trivial switch statements Note

This switch statement should either handle more cases, or be rewritten as an if statement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants