Demoting some of the non-warning messages to notice#10715
Demoting some of the non-warning messages to notice#10715oranagra merged 6 commits intoredis:unstablefrom
Conversation
|
@madolson please comment on the cluster related log prints. @yossigo please approve. i understand you consider that a possibly breaking change so we'll list it in the release notes, but other than that, i imagine we can merge it? |
|
i said i'm unsure about these, not that you should revert them.. please don't resolve the comments and let's see what feedback we get. |
|
sorry, after a re-thinking, i changed it. i did keep the comment open |
|
discussed with Yossi, since this could be breaking change (for someone parsing the log file), and the value it brings is low, we decided to push this back to 7.2 |
|
What are the tenets for setting the value of the different log levels? Are we recategorizing warnings to be "errors" and everything else goes down to notice? |
|
@madolson yes, when we started coloring warnings in red in the other PR, it looked odd since some are not indicating any error. the other alternative was to introduce a new LL_ERROR, and promote the errors to that one, but it seems it'll require more changes, and also could be a greater breaking change for both modules and anyone who's parsing this file. |
|
Alright, the ones in cluster.c look all correct, I also double checked all the existing cases. I agree, I don't think we need an ERROR. |
|
@madolson by correct you mean the current version of the PR? |
|
@enjoy-binbin Are you going to revive and merge this? I think we kind of dropped this. |
|
@madolson i updated it and took a quick look with all the new code, it is now the final version. please re-review |
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
everything outside cluster.c and sentinel.c looks good to me.
madolson
left a comment
madolson
left a comment
There was a problem hiding this comment.
Re-reviewed the cluster stuff and it all still looks ok.
|
@soloestoy @yossigo Do one of you two have enough context to review the sentinel logs? |
|
@moticless can you ack that the changes to sentinel make sense (the log messages that were demoted are just info on normal behaviour and not an indication of an error) |
|
Sentinel LGTM. Thanks. |
We have cases where we print information (might be important but by no means an error indicator) with the LL_WARNING level. Demoting these to LL_NOTICE: - oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo - User requested shutdown... This is also true for cases that we encounter a rare but normal situation. Demoting to LL_NOTICE. Examples: - AOF was enabled but there is already another background operation. An AOF background was scheduled to start when possible. - Connection with master lost. base on yoav-steinberg's redis#10650 (comment) and yossigo's redis#10650 (review)
We have cases where we print information (might be important but by no means an error indicator) with the LL_WARNING level. Demoting these to LL_NOTICE: - oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo - User requested shutdown... This is also true for cases that we encounter a rare but normal situation. Demoting to LL_NOTICE. Examples: - AOF was enabled but there is already another background operation. An AOF background was scheduled to start when possible. - Connection with master lost. base on yoav-steinberg's redis#10650 (comment) and yossigo's redis#10650 (review)
5 participants
We have cases where we print information (might be important but by no means an error indicator) with the LL_WARNING level. Maybe we should demote these to LL_NOTICE:
This is also true for cases that we encounter a rare but normal situation. Maybe here we should too demote to LL_NOTICE. Examples:
base on yoav-steinberg's #10650 (comment) and yossigo's #10650 (review)