Buffer fixes #223

frikilax · 2021-05-17T18:07:55Z

✨ Buffer fixes and improvements

📃 Type of change

Please delete options that are not relevant.

Bug fix: non-breaking change which fixes an issue.

💡 Related Issue(s)

Resolve [IDEA] buffer filter should set an expiration on redis lists #212

✒️ Description

Fixed

[BUFFER] Filter crashed when source wasn't a string
[BUFFER] Filter crashed sometimes with multiple outputs
[BUFFER] The sum filter did not work for "zero-length" cache
[BUFFER/UNAD] Wait for interval before launching first thread functions

Added

[BUFFER] Expire cache entries to prevent leftovers in Redis

🎯 Test Environments

Ubuntu (18.04)

Redis (4.0.9)

✔️ Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
(If other changes) I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

🙋 I certify on my honor that all the information provided is true, and I've done all I can to deliver a high quality code

samples/fbuffer/Connectors/AConnector.cpp

ThibaudCartegnie · 2022-01-12T09:52:56Z

samples/fbuffer/Connectors/AConnector.cpp

+
+    if(redis.Query(std::vector<std::string>{"EXPIRE", key, std::to_string(expiry)}, result, true) != REDIS_REPLY_INTEGER) {
+        DARWIN_LOG_ERROR("AConnector::REDISSetExpiry:: not the expected Redis response");
+        return -1;


implicit cast, this should return false

ThibaudCartegnie · 2022-01-12T10:01:58Z

toolkit/AThread.cpp

+                _is_stop = true;
+                break;
+            }
+        }


I see the logic is mostly hte same, I don't understand the need for a change in this method

the change is in the position of the this->_cv.wait_for()
While we immediately started the task before, now it waits one iteration of _interval before launching it

ThibaudCartegnie · 2022-01-12T10:08:26Z

samples/fbuffer/Connectors/SumConnector.cpp

-        DARWIN_LOG_INFO("SumConnector:: REDISPopLogs:: key '" + sum_name + "' does not exist (yet?)");
-        return false;
+        DARWIN_LOG_DEBUG("SumConnector:: REDISPopLogs:: key '" + sum_name + "' did not exist");
+        result = std::string("0");


This modification seems to be done to resolve an issue that I don't know nor can identify the issue, I can't review it

When Redis returns a NIL, it means that the key does not exist
For our use, the key might not exist yet, so this is not an invalid read (so we mustn't return false)

However, I can't remember why I casted the value to std::string("0") here, but I will improve the code

ThibaudCartegnie · 2022-01-12T10:08:42Z

samples/fbuffer/Connectors/SumConnector.cpp

@@ -163,7 +162,7 @@ long long int SumConnector::REDISListLen(const std::string &sum_name) noexcept {
    }
    else {
        DARWIN_LOG_ERROR("SumConnector::REDISListLen:: Error while querying key '" + sum_name + "'");
-        result = 0.0L;
+        return -1;


This modification seems to be done to resolve an issue that I don't know nor can identify the issue, I can't review it

the previous return didn't specify an error in the process, now it does (and is checked in BufferThread.cpp:34)

automatically set an expiration time (interval time + 60 seconds) to each cache key this avoids changing a buffer filter's configuration and having keys left unused on Redis

…ot exist

…artup

…pLogs

…ring return on REDISSetExpiry

…t on a key

frikilax self-assigned this May 17, 2021

frikilax linked an issue Jun 8, 2021 that may be closed by this pull request

[IDEA] buffer filter should set an expiration on redis lists #212

Open

ThibaudCartegnie requested changes Jan 12, 2022

View reviewed changes

frikilax force-pushed the buffer_fixes branch from f5246e1 to af58aa4 Compare February 4, 2022 10:20

frikilax added 8 commits June 3, 2022 17:40

BUFFER::FIXED:: Filter crashed when source wasn't a string

eede800

BUFFER::FIXED:: Filter crashing sometimes with multiple outputs

b569c2d

BUFFER::ADDED:: Expire buffering keys to avoid leftovers

52424d5

automatically set an expiration time (interval time + 60 seconds) to each cache key this avoids changing a buffer filter's configuration and having keys left unused on Redis

BUFFER::FIXED:: The sum filter type returns 0 (zero) when list does n…

521a5d5

…ot exist

ATHREAD::FIXED:: Do not immediately run buffer/tanomaly threads at st…

3151db3

…artup

SUMCONNECTOR::CHANGED:: Improve readability and formatting on REDISPo…

c054068

…pLogs

ACONNECTOR::CHANGED:: set default value to 'result' and avoid cast du…

e25569b

…ring return on REDISSetExpiry

BUFFER::ADDED:: Generate a warning in logs when expiry couldn't be se…

62bdff2

…t on a key

frikilax force-pushed the buffer_fixes branch from af58aa4 to 62bdff2 Compare June 3, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer fixes #223

Buffer fixes #223

frikilax commented May 17, 2021 •

edited

ThibaudCartegnie Jan 12, 2022

ThibaudCartegnie Jan 12, 2022

frikilax Feb 4, 2022 •

edited

ThibaudCartegnie Jan 12, 2022

frikilax Feb 4, 2022

ThibaudCartegnie Jan 12, 2022

frikilax Feb 4, 2022

Buffer fixes #223

Are you sure you want to change the base?

Buffer fixes #223

Conversation

frikilax commented May 17, 2021 • edited

✨ Buffer fixes and improvements

📃 Type of change

💡 Related Issue(s)

✒️ Description

Fixed

Added

🎯 Test Environments

Ubuntu (18.04)

✔️ Checklist:

ThibaudCartegnie Jan 12, 2022

Choose a reason for hiding this comment

ThibaudCartegnie Jan 12, 2022

Choose a reason for hiding this comment

frikilax Feb 4, 2022 • edited

Choose a reason for hiding this comment

ThibaudCartegnie Jan 12, 2022

Choose a reason for hiding this comment

frikilax Feb 4, 2022

Choose a reason for hiding this comment

ThibaudCartegnie Jan 12, 2022

Choose a reason for hiding this comment

frikilax Feb 4, 2022

Choose a reason for hiding this comment

frikilax commented May 17, 2021 •

edited

frikilax Feb 4, 2022 •

edited