Improve error handling and logging for Retryable IO Errors

[VasuDevrani]

Search before asking

• I had searched in the issues and found no similar issues.

Motivation

In server.cc file, the code checks every minute if the storage is in a retryable IO error state and resumes the database if necessary.

 if (counter != 0 && counter % 600 == 0 && storage->IsDBInRetryableIOError()) {
    storage->GetDB()->Resume();
    LOG(INFO) << "[server] Schedule to resume DB after retryable IO error";
    storage->SetDBInRetryableIOError(false);
  }

Concerns:

1. The code resumes the database and sets the retryable IO error state to false without checking if the resume operation itself succeeded.
2. logs INFO message which may be ERROR or WARNING instead

Solution

example code to resolve this:

if (counter != 0 && counter % 600 == 0 && storage->IsDBInRetryableIOError()) {
  auto status = storage->GetDB()->Resume();
  if (status.ok()) {
    LOG(WARNING) << "[server] Successfully resumed DB after retryable IO error: " << status.ToString();
    storage->SetDBInRetryableIOError(false);
  } else {
    LOG(ERROR) << "[server] Failed to resume DB after retryable IO error: " << status.ToString();
    // Additional error handling, such as retrying or notifying the administrator
  }
}

Are you willing to submit a PR?

• I'm willing to submit a PR!

[git-hulk]

@VasuDevrani Looks good, you can go ahead to improve this.

[VasuDevrani]

@VasuDevrani Looks good, you can go ahead to improve this.

See, the code proposed as solution just prints ERROR in else block.
How about we add a set number of retry for resuming DB operation in case of failure, and if it still fails after retries, then we log a CRITICAL error finally.

[git-hulk]

I'm wondering if it's a good idea to do that. I prefer letting users determine whether to terminate themself instead of by default N times retry. @PragmaTwice @caipengbo @torwig What do you think?

[caipengbo]

Yes, I also don't think it's necessary to set a number of retries. Alternatively, we could provide a RESUME command, which would give administrators more options.

[mapleFU]

Out-of curiousity, do we have ability to recovery from error if Resume() failed for one-times? Seems we can only recover from merely case like Compaction output error or flush error?

[caipengbo]

do we have ability to recovery from error if Resume() failed for one-times?

There might be some cases (disk no space?) where it needs to be done externally for the Resume() to succeed.

[mapleFU]

Thats a good point, so we should let user decide what to do during this case...

[git-hulk]

@VasuDevrani As discussed above, improving the logging message is good, but don't expect to escalate the FATAL error after N time tries. What do you think?

[VasuDevrani]

@VasuDevrani As discussed above, improving the logging message is good, but don't expect to escalate the FATAL error after N time tries. What do you think?

Yeah I've considered above conversation. I'm learning a bit more about Kvrocks, its internals. will update here with my thoughts after a while. Thanks for following up.

[git-hulk]

@VasuDevrani Cool, don't hesitate to raise if you have any ideas.

[VasuDevrani]

@VasuDevrani As discussed above, improving the logging message is good, but don't expect to escalate the FATAL error after N time tries. What do you think?

I agree with just improving the logging message for now. (should i make continue with a PR?)

@VasuDevrani Cool, don't hesitate to raise if you have any ideas.

Later, if need arise we can go with idea of having RESUME command for admins as suggested by @caipengbo
or, we can add a configuration option for max_number_of_retry for this operation

[git-hulk]

I agree with just improving the logging message for now. (should i make continue with a PR?)

Sure, you can continue improving the logging message.

[caipengbo]

I agree with just improving the logging message for now. (should i make continue with a PR?)

Yes, RESUME is not a high priority.