-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add RedisClient class and ScopeLogger class #2
Conversation
i remove the previous commit. so you could create a new pull request with just new files. |
@@ -27,6 +29,19 @@ class Logger | |||
static Logger &getInstance(); | |||
static void setMinPrio(Priority prio); | |||
void write(Priority prio, const char *fmt, ...); | |||
|
|||
class ScopeLogger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why define this inside the Logger class? why not outside of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is an issue to fully separate it from logger class since they both depend on each oder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a very good patch, however, as Guohan wrote, I don't think it should be within Logger class.
In addition, it would be nice to expand this patch and include thread-id
|
||
RedisClient(swss::DBConnector *db); | ||
|
||
int64_t del(std::string key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why we need this class at all. We have started to implement generic function under Table.h.
Most of the suggested function are already implemented there (In your previous commit):
Table::del
Table::delField
Table::setField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table class will not provide all functionality that i need, and those functions from table will be removed later on
Help resolve review comments
Minor updates only
No description provided.