Fixes #5931 - SslConnection should implement getBytesIn()/getBytesOut(). #6335
Conversation
Signed-off-by: Simone Bordet <[email protected]>
Make sure the connection is closed before the assertions. Signed-off-by: Simone Bordet <[email protected]>
Fixed CountDownLatch count. Signed-off-by: Simone Bordet <[email protected]>
Fixed CountDownLatch count again. Signed-off-by: Simone Bordet <[email protected]>
| private final List<SslHandshakeListener> handshakeListeners = new ArrayList<>(); | ||
| private final AtomicLong _bytesIn = new AtomicLong(); |
There was a problem hiding this comment.
We have been changing to LongAdder in other stats classes, so perhaps we should use here as well?
There was a problem hiding this comment.
I think LongAdder is more suited when it's contended, but here is not so I went for AtomicLong.
|
@sbordet is there any issue that existing usages of |
|
@lachlan-roberts will the double counting happen? Doesn't |
| public void testBytesInBytesOut() throws Exception | ||
| { | ||
| // Two connections will be closed: SslConnection and HttpConnection. | ||
| // Two on the server, two on the client. |
There was a problem hiding this comment.
@gregw if you look at the test here it seems to indicate that both SslConnection and HttpConnection are recorded by ConnectionStatistics.
ConnectionStatistics works as a Connection.Listener so it would apply to SslConnection as well.
There was a problem hiding this comment.
Hmmm is this different on the Server-side? As I don't see how a Connection.Listener would see any connections wrapped by SslConnection?
There was a problem hiding this comment.
The Connection.Listener is added as a bean to all connectors on the server, and any Connection.Listener beans on the Connector are added to each new connection that is created. So the listener will automatically be added to the SslConnection.
There was a problem hiding this comment.
Yeah, so the listener is on SslConnection. Is it also inherited by the Wrapped HttpConnection? If so, then i agree the double counting is wrong.
Updated ConnectionStatistics to report both the stats of all connections, and the stats grouped by connection class. Signed-off-by: Simone Bordet <[email protected]>
|
@sbordet you forgot to request re-review! I've been waiting for y our checkin! |
|
|
||
| _connections.increment(); | ||
| _stats.incrementCount(); | ||
| _statsMap.computeIfAbsent(connection.getClass().getName(), Stats::new).incrementCount(); |
There was a problem hiding this comment.
perhaps delegate the computeIfAbsent to a protected method, so that if somebody really wanted to pick and choose which types get counted then they could override that method and only return a Stats if they wanted that specific connection counted and it might be on a different criteria than className.
| } | ||
|
|
||
| @Override | ||
| public void onClosed(Connection connection) | ||
| { | ||
| if (!isStarted()) | ||
| return; | ||
| onClosed(_stats, connection); | ||
| Stats stats = _statsMap.get(connection.getClass().getName()); |
There was a problem hiding this comment.
Ditto. Put this get call into a protected method so selection can be done on criteria other than class name.
Updates after review. Signed-off-by: Simone Bordet <[email protected]>
…(). (#6335) Updated ConnectionStatistics to report both the stats of all connections, and the stats grouped by connection class. Signed-off-by: Simone Bordet <[email protected]> (cherry picked from commit f902d12)
…(). (#6335) Updated ConnectionStatistics to report both the stats of all connections, and the stats grouped by connection class. Signed-off-by: Simone Bordet <[email protected]> (cherry picked from commit f902d12)
Signed-off-by: Simone Bordet [email protected]