Added support for setting TTL on a Table managed entry. by Cosmin-Jinga-MS · Pull Request #526 · sonic-net/sonic-swss-common

Cosmin-Jinga-MS · 2021-09-06T06:44:28Z

[table.h/rediscommand.h]

Updated table.h with a new set() routine capable of also setting TTL on an entry.
Updated rediscommand.h with the EXPIRE command

Signed-off-by: Cosmin Jinga [email protected]

…aged entry. * Updated table.h with a new set() routine capable of also setting TTL on an entry. * Updated rediscommand.h with the EXPIRE command Signed-off-by: Cosmin Jinga <[email protected]>

ghost · 2021-09-06T06:44:39Z

All CLA requirements met.

TACappleman · 2021-09-07T14:42:52Z

@qiluo-msft could you review this PR please?

kcudnik · 2021-09-07T14:54:00Z

common/table.cpp

 }

+void Table::set(const string &key, const vector<FieldValueTuple> &values,
+                const int32_t &ttl, const string& /*op*/, const string& /*prefix*/)


why op and prefix are omitted ? set should work exactly the same as with or without ttl, otherwise should be named differently

op and prefix are also omitted in the existing set() routine for Table
I've overloaded set() with the extra ttl argument to have a distinction between it and the existing one.
I'm adding a reworked version of the code with the ttl is set directly in the existing routine.

@qiluo-msft what do you think here?

common/table.h

qiluo-msft · 2021-09-08T07:57:35Z

common/rediscommand.h

    void formatHDEL(const std::string& key, const std::vector<std::string>& fields);

+    /* Format EXPIRE key field command */
+    void formatEXPIRE(const std::string& key, const int32_t& ttl);


const int32_t&

int is good enough.
How do you know it's 32 bits? It seems supporting 64 bits. #Closed

Agreed, changing the type to int.

Is int64_t better?

Redis documentation on expire does not actually provide a range for the ttl values https://redis.io/commands/expire
Having manually tried an in-range and an out-of-range int64 value it does indeed need to be int64.

127.0.0.1:6379> EXPIRE ISIS_ROUTER_LSP_STATE:localhost:1 9223372036854775801
(integer) 1
127.0.0.1:6379> EXPIRE ISIS_ROUTER_LSP_STATE:localhost:1 9223372036854775809
(error) ERR value is not an integer or out of range
I'll update the code momentarily.

Updated code to integrate ttl configuration into existing set() routine, thus preventing code duplication.

Updated set() header to allow ttl configuration. Moved DEFAULT_DB_TTL define outside of Table class for it to be visible in test code.

Changed ttl type to int

Cosmin-Jinga-MS

Addressed comments;
Main update is removal of the new overloaded set() and moving of the ttl configuration inside the existing routine.

Changed to int64 to fit full range of redis accepted input.

Changing set() ttl param placement to fix compilation error.

Changing ttl param order in set() to fix compilation errors

Changed order of ttl param to fix compilation error

Cosmin-Jinga-MS

Updated ttl to int64_type and changed its position in the set() params to fix overlooked compilation errors in existing calls for set()

qiluo-msft · 2021-09-09T05:50:13Z

common/rediscommand.cpp

+/* Format EXPIRE key field command */
+void RedisCommand::formatEXPIRE(const std::string& key, const int64_t& ttl)
+{
+    return format("EXPIRE %s %d", key.c_str(), ttl);


%d

hiredis internally implement some int length. ref: https://github.com/redis/hiredis/blob/2d9d77518d012a54ae34f9822e4b4d19823c4b75/hiredis.c#L434

Seems like %lld is for int64_t. Could you check and add unit test? #Closed

yes, I will update it to %lld and add/extend an UT to also issue a set with configured TTL

return in void function? formta() also is void?

qiluo-msft · 2021-09-09T05:53:36Z

common/table.cpp

+
+      if (!m_buffered)
+      {
+          m_pipe->flush();


flush

Is it possible to flush only once in this function? #Closed

Does RedisPipeline support enqueuing of more than one command without it being flushed inbetween?
i.e.:

cmd.formatHMSET(getKeyName(key), values.begin(), values.end()); m_pipe->push(cmd, REDIS_REPLY_STATUS); cmd.formatEXPIRE(getKeyName(key), ttl); m_pipe->push(cmd, REDIS_REPLY_INTEGER); if (!m_buffered) { m_pipe->flush(); }

If it does the code can be changed to:

if (ttl != DEFAULT_DB_TTL) { // Configure the expire time for the entry that was just added cmd.formatEXPIRE(getKeyName(key), ttl); m_pipe->push(cmd, REDIS_REPLY_INTEGER); if (!m_buffered) { m_pipe->flush(); } } else { if (!m_buffered) { m_pipe->flush(); } }

By design, yes. In your proposed code, you can further improve by move flush out of if-else.

Cosmin-Jinga-MS · 2021-09-09T06:54:20Z

@qiluo-msft Updating set() with an extra param for ttl rather than overloading it has raised compilation dependencies in sonic-swss's mock_table.cpp which I've missed during the local build of swss-common but have been uncovered by the pipeline now.
What would be the best approach here? Update sonic-swss as well with the extra ttl param? Not sure how to integrate it in the same PR as it's from a different submodule.
Should I revert back to overloading set()?

qiluo-msft · 2021-09-09T07:11:14Z

I see this is painful. Seems overloading set() is better approach. If you choose this way, please make sure

reuse code as much as possible, for example, rewrite old function by calling your new function
Add TODO comment to remove the overloading and use default parameter

In reply to: 915814243

Updated logic to call pipeline->flush() only once

Updated format type to long long int.

qiluo-msft · 2021-09-09T11:26:05Z

common/table.cpp

+    if (ttl != DEFAULT_DB_TTL)
+    {
+      // Configure the expire time for the entry that was just added
+      cmd.formatEXPIRE(getKeyName(key), ttl);


[](http://example.com/codeflow?start=0&length=6)

Please indent with 4x spaces #Closed

updated indents in last commit

… as expected

Cosmin-Jinga-MS

Added new set() as the driving routine for set OPs

Cosmin-Jinga-MS · 2021-09-14T08:16:12Z

Hi @qiluo-msft,
About the remaining failing UT from the current pipeline. https://dev.azure.com/mssonic/build/_build/results?buildId=35308&view=ms.vss-test-web.build-test-results-tab
I'm not seeing anything wrong with the set operation in it(it calls the old set() implementation, nothing touches the new ttl code in this scenario) and it seems it's not a particularly stable UT(looking into its history)

Is there a chance a retry of this pipeline would fix it?(I don't have enough rights on my account to re-run failed UTs from the gitlab pipeline interface)

TACappleman · 2021-09-14T08:18:36Z

/azpw run

qiluo-msft · 2021-09-14T09:22:00Z

The PR owner could use /azpw run to retrigger checkers.

Cosmin-Jinga-MS · 2021-09-14T09:44:20Z

/azpw run

mssonicbld · 2021-09-14T09:44:21Z

/AzurePipelines run

azure-pipelines · 2021-09-14T09:44:35Z

Azure Pipelines successfully started running 1 pipeline(s).

Cosmin-Jinga-MS · 2021-09-14T12:56:14Z

Looks like the failure is still present.
I've ran the UT in question locally and managed to repro the issue both with my changes from swss-common in place and with a fresh build from master. Currently looking a bit at the failure to see if I can spot the problem but it does not look to be caused by my changes.
Syslog err output from the sonic-swss-common master run(without my changes):

Looks like a failure to program those interfaces due to confilct on the lane/ids to me which leads to a orchagent crash.

Cosmin-Jinga-MS · 2021-09-15T07:20:34Z

@qiluo-msft
I did not manage to get very far with the investigation into this as orchagent logs are rate-limited before the failure occurs once it's set to DEBUG level. Are there any overnight/on-demand runs we can check to confirm this tests failure reproduces on master as well?(as said before, I'm seeing this failure on my local master ut run too)
If it does repro on a master pipeline as well, it would be better to defer the investigation to someone more familiar with recirc ports, in my opinion.

qiluo-msft · 2021-09-15T07:24:57Z

You can check master branch, other branches or PR on https://dev.azure.com/mssonic/build/_build?definitionId=9&_a=summary&view=branches

I just trigged a new build on master branch.

TACappleman · 2021-09-15T08:22:26Z

One of the ones that's failed on the latest run fails on a large percentage of pipelines, and the other one sometimes, so worth keeping on trying until it works

Cosmin-Jinga-MS · 2021-09-15T08:51:28Z

/azpw run

mssonicbld · 2021-09-15T08:51:29Z

/AzurePipelines run

azure-pipelines · 2021-09-15T08:51:41Z

Azure Pipelines successfully started running 1 pipeline(s).

Cosmin-Jinga-MS · 2021-09-15T11:10:05Z

@qiluo-msft I see the master run has also failed on the test_recirc_port UT(it's now running a 2nd attempt). My PR retry also failed that UT again. Should I keep retrying too?

Cosmin-Jinga-MS · 2021-09-15T12:31:42Z

/azpw run

mssonicbld · 2021-09-15T12:31:43Z

/AzurePipelines run

azure-pipelines · 2021-09-15T12:31:56Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2021-09-19T02:56:25Z

/azp run

azure-pipelines · 2021-09-19T02:56:37Z

Azure Pipelines successfully started running 1 pipeline(s).

[table.h/rediscommand.h] * Updated table.h with a new set() routine capable of also setting TTL on an entry. * Updated rediscommand.h with the EXPIRE command

Added support for setting TTL on a Table managed entry. (sonic-net#526)

The following fixes are included: b502743 [gearbox] Since ASIC_DB, as well its COUNTER_DB, FLEX_COUNTER_DB use separator ':', GB_ASIC_DB should use same ((sonic-net/sonic-swss-common#532) 81182ec Added support for setting TTL on a Table managed entry. ((sonic-net/sonic-swss-common#526)

[table.h/rediscommand.h] * Updated table.h with a new set() routine capable of also setting TTL on an entry. * Updated rediscommand.h with the EXPIRE command

…onic-net#526)

[table.h/rediscommand.h] Added support for setting TTL on a Table man…

3bd9672

…aged entry. * Updated table.h with a new set() routine capable of also setting TTL on an entry. * Updated rediscommand.h with the EXPIRE command Signed-off-by: Cosmin Jinga <[email protected]>

Cosmin-Jinga-MS marked this pull request as draft September 6, 2021 06:46

Cosmin-Jinga-MS marked this pull request as ready for review September 6, 2021 06:50

Cosmin-Jinga-MS closed this Sep 6, 2021

Cosmin-Jinga-MS reopened this Sep 6, 2021

kcudnik reviewed Sep 7, 2021

View reviewed changes

common/table.h Outdated Show resolved Hide resolved

qiluo-msft reviewed Sep 8, 2021

View reviewed changes

Cosmin-Jinga-MS added 4 commits September 8, 2021 15:52

Update table.cpp

55a14f4

Updated code to integrate ttl configuration into existing set() routine, thus preventing code duplication.

Update table.h

93bafee

Updated set() header to allow ttl configuration. Moved DEFAULT_DB_TTL define outside of Table class for it to be visible in test code.

Update rediscommand.h

9292af7

Changed ttl type to int

Update rediscommand.cpp

9d2f6bb

Changed ttl type to int

Cosmin-Jinga-MS commented Sep 8, 2021

View reviewed changes

Cosmin-Jinga-MS added 5 commits September 9, 2021 08:13

Update rediscommand.cpp

1ef8da5

Changed to int64 to fit full range of redis accepted input.

Update rediscommand.h

1322886

Changed to int64 to fit full range of redis accepted input.

Update table.cpp

9dca57e

Changing set() ttl param placement to fix compilation error.

Update table.h

0ffe760

Changing ttl param order in set() to fix compilation errors

Update table.h

e6ac594

Changed order of ttl param to fix compilation error

Cosmin-Jinga-MS commented Sep 9, 2021

View reviewed changes

qiluo-msft reviewed Sep 9, 2021

View reviewed changes

Cosmin-Jinga-MS added 2 commits September 9, 2021 10:12

Update table.cpp

46cf41e

Updated logic to call pipeline->flush() only once

Update rediscommand.cpp

dd19972

Updated format type to long long int.

qiluo-msft reviewed Sep 9, 2021

View reviewed changes

Reverted to overloaded set() routine and added UT to check ttl is set…

7304f02

… as expected

Cosmin-Jinga-MS force-pushed the expire_ttl_in_Table branch from 3d32f3c to 7304f02 Compare September 9, 2021 13:08

Updated new overloaded set() as the driver routine for set operations

da3a88c

Cosmin-Jinga-MS commented Sep 10, 2021

View reviewed changes

qiluo-msft approved these changes Sep 13, 2021

View reviewed changes

qiluo-msft merged commit 81182ec into sonic-net:master Sep 19, 2021

Cosmin-Jinga-MS added a commit to Metaswitch/sonic-swss-common that referenced this pull request Sep 20, 2021

Merge pull request #7 from Metaswitch/metaswitch/expire_ttl_in_Table

f2eeb20

Added support for setting TTL on a Table managed entry. (sonic-net#526)

prgeor pushed a commit to prgeor/sonic-swss-common that referenced this pull request Feb 27, 2025

[cmis] Optimize cmis.get_error_description speed for passive module (s…

2c0f9ed

…onic-net#526)

Conversation

Cosmin-Jinga-MS commented Sep 6, 2021

Uh oh!

ghost commented Sep 6, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TACappleman commented Sep 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qiluo-msft Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cosmin-Jinga-MS Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cosmin-Jinga-MS left a comment

Choose a reason for hiding this comment

Uh oh!

Cosmin-Jinga-MS left a comment

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cosmin-Jinga-MS commented Sep 9, 2021

Uh oh!

qiluo-msft commented Sep 9, 2021

Uh oh!

qiluo-msft Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cosmin-Jinga-MS left a comment

Choose a reason for hiding this comment

Uh oh!

Cosmin-Jinga-MS commented Sep 14, 2021

Uh oh!

TACappleman commented Sep 14, 2021

Uh oh!

qiluo-msft commented Sep 14, 2021

Uh oh!

Cosmin-Jinga-MS commented Sep 14, 2021

Uh oh!

mssonicbld commented Sep 14, 2021

Uh oh!

azure-pipelines bot commented Sep 14, 2021

Uh oh!

Cosmin-Jinga-MS commented Sep 14, 2021

Uh oh!

Cosmin-Jinga-MS commented Sep 15, 2021

Uh oh!

qiluo-msft commented Sep 15, 2021

Uh oh!

ghost commented Sep 6, 2021 •

edited by ghost

Loading

qiluo-msft Sep 8, 2021 •

edited

Loading

Cosmin-Jinga-MS Sep 9, 2021 •

edited

Loading

qiluo-msft Sep 9, 2021 •

edited

Loading

qiluo-msft Sep 9, 2021 •

edited

Loading

qiluo-msft Sep 9, 2021 •

edited

Loading