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.

Sign up for GitHub

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
Open

log error when ReadEntryProcessor IOException #4199

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b81662
Change log level to error when ReadEntryProcessor IOException
AnonHxy Feb 4, 2024
0eaf8ab
Optimize
AnonHxy Feb 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,7 @@ protected void processPacket() {
}
errorCode = BookieProtocol.ENOENTRY;
} catch (IOException e) {
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:

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java

Lines 249 to 251 in 34bd4b3

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.

errorCode = BookieProtocol.EIO;
} catch (BookieException.DataUnknownException e) {
LOG.error("Ledger {} is in an unknown state", request.getLedgerId(), e);
Expand Down
Loading