Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Roger,
Thanks for reviewing.
Thank you
-Hamlin
On 2019/11/27 11:09 PM, Roger Riggs wrote:
Looks good.
Thanks for the revisions, Roger
On 11/26/19 8:51 PM, Hamlin Li wrote:
Hi Roger,
Thanks for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/27 2:47 AM, Roger Riggs wrote:
Hi Hamlin,
ok, but it looses the logging of the connection close when the
socket is null.
I intended to suggest that the logging happened before/outside the test
for socket != null.
if (TCPTransport.tcpLog.isLoggable(Log.BRIEF)) {
TCPTransport.tcpLog.log(Log.BRIEF,"close connection, socket: " +
socket);
}if (socket != null) { Thanks, Roger
On 11/22/19 2:54 AM, Hamlin Li wrote:
Hi Roger,
Thank you for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/18 11:48 PM, Roger Riggs wrote:
Hi Hamlin,
TCPConnection.java:212:
Keep the "close connection" logging and add the socket to the same
log message:
If anyone is scraping the log, they won't loose this message.
TCPTransport.tcpLog.log(Log.BRIEF, "close connection, socket: " +
socket);
TCPTransport.java
277-278: combine the message to be one logging call.
server socket
289: use Log.BRIEF, avoid creating mixture of and too many log
levels.
Reword the log messages so they each begin with "server socket...",
or "server socket close"...
it makes it easier to grep for and coorelate related messages
Thanks, Roger
On 11/6/19 7:02 AM, Hamlin Li wrote:
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server
socket on " + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method
and deal with it in two places:
- in exportObject() - add it as suppressed exception to
exception thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into
UncheckedIOException (depending on what are the callers of
targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to
catch block, as I think it's simple/straight and sufficient to
help diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket
closing, this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Looks good.
Thanks for the revisions, Roger
On 11/26/19 8:51 PM, Hamlin Li wrote:
Hi Roger,
Thanks for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/27 2:47 AM, Roger Riggs wrote:
Hi Hamlin,
ok, but it looses the logging of the connection close when the socket
is null.
I intended to suggest that the logging happened before/outside the test
for socket != null.
if (TCPTransport.tcpLog.isLoggable(Log.BRIEF)) {
TCPTransport.tcpLog.log(Log.BRIEF,"close connection, socket: " + socket);
}if (socket != null) { Thanks, Roger
On 11/22/19 2:54 AM, Hamlin Li wrote:
Hi Roger,
Thank you for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/18 11:48 PM, Roger Riggs wrote:
Hi Hamlin,
TCPConnection.java:212:
Keep the "close connection" logging and add the socket to the same
log message:
If anyone is scraping the log, they won't loose this message.
TCPTransport.tcpLog.log(Log.BRIEF, "close connection, socket: " +
socket);
TCPTransport.java
277-278: combine the message to be one logging call.
server socket
289: use Log.BRIEF, avoid creating mixture of and too many log levels.
Reword the log messages so they each begin with "server socket...",
or "server socket close"...
it makes it easier to grep for and coorelate related messages
Thanks, Roger
On 11/6/19 7:02 AM, Hamlin Li wrote:
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server
socket on " + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method
and deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into
UncheckedIOException (depending on what are the callers of
targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help
diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket
closing, this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Roger,
Thanks for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/27 2:47 AM, Roger Riggs wrote:
Hi Hamlin,
ok, but it looses the logging of the connection close when the socket
is null.
I intended to suggest that the logging happened before/outside the test
for socket != null.
if (TCPTransport.tcpLog.isLoggable(Log.BRIEF)) {
TCPTransport.tcpLog.log(Log.BRIEF,"close connection, socket: " + socket);
}if (socket != null) { Thanks, Roger
On 11/22/19 2:54 AM, Hamlin Li wrote:
Hi Roger,
Thank you for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/18 11:48 PM, Roger Riggs wrote:
Hi Hamlin,
TCPConnection.java:212:
Keep the "close connection" logging and add the socket to the same
log message:
If anyone is scraping the log, they won't loose this message.
TCPTransport.tcpLog.log(Log.BRIEF, "close connection, socket: " +
socket);
TCPTransport.java
277-278: combine the message to be one logging call.
server socket
289: use Log.BRIEF, avoid creating mixture of and too many log levels.
Reword the log messages so they each begin with "server socket...",
or "server socket close"...
it makes it easier to grep for and coorelate related messages
Thanks, Roger
On 11/6/19 7:02 AM, Hamlin Li wrote:
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server
socket on " + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method
and deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into
UncheckedIOException (depending on what are the callers of
targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help
diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket
closing, this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Hamlin,
ok, but it looses the logging of the connection close when the socket is
null.
I intended to suggest that the logging happened before/outside the test
for socket != null.
if (TCPTransport.tcpLog.isLoggable(Log.BRIEF)) {
TCPTransport.tcpLog.log(Log.BRIEF,"close connection, socket: " + socket);
}if (socket != null) { Thanks, Roger
On 11/22/19 2:54 AM, Hamlin Li wrote:
Hi Roger,
Thank you for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/18 11:48 PM, Roger Riggs wrote:
Hi Hamlin,
TCPConnection.java:212:
Keep the "close connection" logging and add the socket to the same
log message:
If anyone is scraping the log, they won't loose this message.
TCPTransport.tcpLog.log(Log.BRIEF, "close connection, socket: " +
socket);
TCPTransport.java
277-278: combine the message to be one logging call.
server socket
289: use Log.BRIEF, avoid creating mixture of and too many log levels.
Reword the log messages so they each begin with "server socket...",
or "server socket close"...
it makes it easier to grep for and coorelate related messages
Thanks, Roger
On 11/6/19 7:02 AM, Hamlin Li wrote:
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket
on " + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method and
deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into
UncheckedIOException (depending on what are the callers of
targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket
closing, this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Roger,
Thank you for reviewing, I have updated as you suggested:
http://cr.openjdk.java.net/~mli/8232446/webrev.01/
Thank you
-Hamlin
On 2019/11/18 11:48 PM, Roger Riggs wrote:
Hi Hamlin,
TCPConnection.java:212:
Keep the "close connection" logging and add the socket to the same log
message:
If anyone is scraping the log, they won't loose this message.
TCPTransport.tcpLog.log(Log.BRIEF, "close connection, socket: " +
socket);
TCPTransport.java
277-278: combine the message to be one logging call.
server socket
289: use Log.BRIEF, avoid creating mixture of and too many log levels.
Reword the log messages so they each begin with "server socket...",
or "server socket close"...
it makes it easier to grep for and coorelate related messages
Thanks, Roger
On 11/6/19 7:02 AM, Hamlin Li wrote:
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket
on " + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method and
deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into
UncheckedIOException (depending on what are the callers of
targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket
closing, this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Hamlin,
TCPConnection.java:212:
Keep the "close connection" logging and add the socket to the same log
message:
If anyone is scraping the log, they won't loose this message.
TCPTransport.tcpLog.log(Log.BRIEF, "close connection, socket: " + socket);
TCPTransport.java
277-278: combine the message to be one logging call.
server socket
289: use Log.BRIEF, avoid creating mixture of and too many log levels.
Reword the log messages so they each begin with "server socket...",
or "server socket close"...
it makes it easier to grep for and coorelate related messages
Thanks, Roger
On 11/6/19 7:02 AM, Hamlin Li wrote:
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket
on " + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method and
deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into UncheckedIOException
(depending on what are the callers of targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket closing,
this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
On 2019/11/6 5:36 PM, Peter Levart wrote:
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket on
" + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even
better, rearrange for IOException to be thrown from that method and
deal with it in two places:
- in exportObject() - add it as suppressed exception to exception
thrown from super.exportObject().
- in targetUnexported() - log it or wrap it into UncheckedIOException
(depending on what are the callers of targetUnexported())
What do you think?
Thanks Peter.
I agree. I adopt your first suggestion: add log statement to catch
block, as I think it's simple/straight and sufficient to help diagnose.
And I also add log for catch blocks in other close places.
The change is updated in place at:
http://cr.openjdk.java.net/~mli/8232446/webrev.00/
Thank you
-Hamlin
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket closing,
this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
Re: RFR of JDK-8232446: logging enhancement for rmi when socket closed
Hi Hamlin,
in TCPTransport.decrementExportCount():
283 try {
284 if (tcpLog.isLoggable(Log.BRIEF)) {
285 tcpLog.log(Log.BRIEF, "close server socket on
" + ss);
286 }
287 ss.close();
288 } catch (IOException e) {
289 }
...you could add a log statement to the catch block too. Or even better,
rearrange for IOException to be thrown from that method and deal with it
in two places:
- in exportObject() - add it as suppressed exception to exception thrown
from super.exportObject().
- in targetUnexported() - log it or wrap it into UncheckedIOException
(depending on what are the callers of targetUnexported())
What do you think?
Regards, Peter
On 11/6/19 3:07 AM, Hamlin Li wrote:
Would you please review the patch?
bug: https://bugs.openjdk.java.net/browse/JDK-8232446
webrev: http://cr.openjdk.java.net/~mli/8232446/webrev.00/
We have some intermittent failures in rmi related to socket closing,
this is to add more logging to help diagnose the issues.
Thanks you
-Hamlin
