Add remote Host info to PageTransportErrorException #4511
Conversation
| long token = getToken(response); | ||
| long nextToken = getNextToken(response); | ||
| boolean complete = getComplete(response); | ||
| String taskInstanceId = getTaskInstanceId(response, uri); |
There was a problem hiding this comment.
do we need to pass uri to all those methods? We are catching PageTransportErrorException below anyway and add remote host information there. IMO that should be eough.
There was a problem hiding this comment.
I didn't do it that way, as that would require creating another constructor of PageTransportErrorException and pass remoteHost as null to its parent. I thought we would not want to do this (pass remoteHost as null for transport exceptions). I will change it if that is okay.
There was a problem hiding this comment.
Yeah... Having remoteHost nullable is kinda ugly. So maybe we can live with this extra parameter passed to those helper methods. @electrum ?
@sanchitkashyap Can you also comment on motivation for the change? The only final PTEE which is sent back to user is constructed in the catch clause below.
And we already have whole uri as part of an exception message there. Is that not enough?
There was a problem hiding this comment.
@losipiuk i have updated the description to explain the motivation.
|
|
||
| import static io.prestosql.spi.StandardErrorCode.PAGE_TRANSPORT_ERROR; | ||
|
|
||
| public class PageTransportErrorException | ||
| extends PrestoException | ||
| extends PrestoTransportException |
There was a problem hiding this comment.
the change here seems not related to Add remote Host info to PageTransportErrorException
is it intentional?
make this into a separate commit
also cc @electrum
There was a problem hiding this comment.
This is needed and related since PrestoTransportException is the base class that contains the remote host, which is extracted in Failures to include as part of the error info. Given that PageTransportTimeoutException already extends it (and is the only subclass), I'm guessing we intended to do it for PageTransportErrorException originally.
|
LGTM. I will let @electrum chime in to comment about making |
|
Thanks! |
This would be useful to identify REMOTE_HOST_GONE for PTEE. For more info look -> prestodb/presto#7691