fix(rdb_load): Handle JSON loading failure when parsing fails #4801
Conversation
src/server/rdb_load.cc
Outdated
| if (json) { | ||
| pv_->SetJson(std::move(*json)); | ||
| } else { | ||
| LOG(ERROR) << "Invalid JSON string"; |
There was a problem hiding this comment.
Won't this trigger errors in productions ? Is that something that we want ? cc @adiholden
There was a problem hiding this comment.
I checked other methods and they log ERROR for similar issues
There was a problem hiding this comment.
we usually print error log for internal error, when we do not expect to have failures.
Here in rdb load its iether that we had bug in snapshot serializing json incorrect or user tries to load some json but we can not parse it.
I do think that its worth to have an error print here. Please also print the blob string here so it will be easier to investigate if we see this error.
How did you get to this error flow stefan?
There was a problem hiding this comment.
Yep I agree with Adi here, it would be useful for us to be aware of those problematic flows
There was a problem hiding this comment.
I didn't reproduce it, but I checked the memory tracking, and during the check, I found out that if an error occurs (so the JSON should be empty), we are still trying to use the parsed value
BagritsevichStepan
force-pushed
the
rdb_loader/fix-json-loading
branch
from
2ab51f8 to
1facbb4
Compare
| @@ -56,10 +56,6 @@ size_t QlMAllocSize(quicklist* ql, bool slow) { | |||
| } | |||
|
|
|||
| size_t UpdateSize(size_t size, int64_t update) { | |||
| if (update >= 0) { | |||
There was a problem hiding this comment.
What does this change fixes?
There was a problem hiding this comment.
We discussed it with Kostas that it can be deleted here. We don't need this check. The method will still work correctly
There was a problem hiding this comment.
@adiholden this should have been removed on the previous PR, but since he had this open, I suggested him to change it here (to avoid reiterating the PR one more time). The removed code is redundant.
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
BagritsevichStepan
force-pushed
the
rdb_loader/fix-json-loading
branch
from
1facbb4 to
cdea355
Compare
| @@ -56,10 +56,6 @@ size_t QlMAllocSize(quicklist* ql, bool slow) { | |||
| } | |||
|
|
|||
| size_t UpdateSize(size_t size, int64_t update) { | |||
| if (update >= 0) { | |||
There was a problem hiding this comment.
@adiholden this should have been removed on the previous PR, but since he had this open, I suggested him to change it here (to avoid reiterating the PR one more time). The removed code is redundant.
Small fix for the case if we cannot parse json string during loading.