Skip to content
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

log error when ReadEntryProcessor IOException #4199

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Feb 4, 2024

Descriptions of the changes in this PR:

Motivation

Change the log-level to error when ReadEntryProcessor throw IOException. The reason is that:

  1. IOException should not happen frequently,so the error log will not print too much
  2. It will be convenient for troubleshooting if we print the error log.
  3. Also we can see that ReadEntryProcessorV3 has printed the error log

} catch (IOException e) {
LOG.error("IOException while reading entry: {} from ledger {} ", entryId, ledgerId, e);
return buildResponse(readResponse, StatusCode.EIO, startTimeSw);
} catch (BookieException.DataUnknownException e) {

Changes

log.debug -> log.error

if (LOG.isDebugEnabled()) {
LOG.debug("Error reading {}", request, e);
}
LOG.error("IOException while reading {}", request, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would print the whole stack trace every time a client gets a connectivity error with any bookie, for each entry that we're trying to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whole stack trace every time a client gets a connectivity error with any bookie

Thanks for your review @merlimat . But I don't get your point. This method runs only on serever side and probability throw IOException if read an entry from storage. So I think it doesn't matter with client print logs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pring ledgerId:entryId instead of the whole request content?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think print whole request is OK. because ReadRequest has overwrite the toString method:

public String toString() {
return String.format("Op(%d)[Ledger:%d,Entry:%d]", opCode, ledgerId, entryId);
}

@hangc0276

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would print the whole stack trace every time a client gets a connectivity error with any bookie, for each entry that we're trying to read.

If the target entry does not exist on the bookie, it will throw NoLedgerException or NoEntryException and won't go into this code path. @merlimat Do you have any concern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would print the whole stack trace every time a client gets a connectivity error with any bookie, for each entry that we're trying to read.

If the target entry does not exist on the bookie, it will throw NoLedgerException or NoEntryException and won't go into this code path. @merlimat Do you have any concern?

The NoLedgerException and NoEntryException extend IOException, it should be caught.

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@hezhangjian
Copy link
Member

@merlimat Could you please take a look again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants