You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "AnonHxy (via GitHub)" <gi...@apache.org> on 2024/02/04 15:22:12 UTC
[PR] log error when ReadEntryProcessor IOException [bookkeeper]
AnonHxy opened a new pull request, #4199:
URL: https://github.com/apache/bookkeeper/pull/4199
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
https://github.com/apache/bookkeeper/blob/dc2bb1dfed38d7f9ad7a29fc92bd907e51af5b21/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java#L233-L236
### Changes
`log.debug` -> `log.error`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#discussion_r1527767782
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -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);
Review Comment:
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#discussion_r1479275766
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -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);
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#discussion_r1499485709
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -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);
Review Comment:
I think print whole `request ` is OK. because `ReadRequest` has overwrite the `toString` method:
https://github.com/apache/bookkeeper/blob/34bd4b38fa13f105608ae4e7b4c749124c271e1e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java#L249-L251
@hangc0276
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#discussion_r1510459788
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -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);
Review Comment:
>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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "shoothzj (via GitHub)" <gi...@apache.org>.
shoothzj commented on PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#issuecomment-2129600965
@merlimat Could you please take a look again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#discussion_r1496806910
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -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);
Review Comment:
Pring `ledgerId:entryId` instead of the whole request content?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] log error when ReadEntryProcessor IOException [bookkeeper]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #4199:
URL: https://github.com/apache/bookkeeper/pull/4199#discussion_r1478770200
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org