Conversation
There was a problem hiding this comment.
Can you review go/java-practices/null from the overall PR's perspective in terms of nullable vs optional ? I am not sure if we have a local java convention to prefer or avoid null
| private final int maxMessageSize; | ||
| private final int maxHeaderListSize; | ||
| private final int softLimitHeaderListSize; | ||
| private MetricRecorder metricRecorder; |
There was a problem hiding this comment.
How is this being set? I don't see a constructor or a setter that accesses this value?
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| final class TcpMetrics { |
There was a problem hiding this comment.
optional: Javadocs for classes might be helpful
| } | ||
|
|
||
| static final class Metrics { | ||
| final LongCounterMetricInstrument connectionsCreated; |
There was a problem hiding this comment.
nullability annotations for fields where needed. Also consider the previous discussion about nullability style guide
| * Safe metric registration or retrieval for environments where TcpMetrics might | ||
| * be loaded multiple times (e.g., shaded and unshaded). | ||
| */ | ||
| private static LongCounterMetricInstrument safelyRegisterLongCounter( |
There was a problem hiding this comment.
Do we need the safelyRegi... private abstraction?
Given that all registration happens during construction and this class is finel , can't we simply ensure we dedupe our metrics during construction, catch and rethrow the error from the constructor? i.e. ensure that the input is valid and catch/throw from a single point which should never be reached.
The fact that "we are passing a valid set of metrics for registration" can probably be trivially unit tested when we construct a TcpMetrics instance.
This should reduce code bloat without any sacrifice to safety, but at the cost of "duplicate input will throw an error instead of being handled intuitively", which is a fair expectation IMO.
| java.util.List<String> labelValues = getLabelValues(channel); | ||
| try { | ||
| if (channel.getClass().getName().equals(epollSocketChannelClassName)) { | ||
| Class<?> tcpInfoClass = Class.forName(epollTcpInfoClassName); |
There was a problem hiding this comment.
This is quite a lot of reflection in hot exporting path. So, a couple of questions.
- Why do we choose reflectin over typecasting and calling actual methods? Reducing dependency surface?
- Is there a way to optimise this like caching value on per channel level? Something like a creating a runnable per channel. Not sure how it'd be possible to share this data for the final collection on channel inactive.
| Method rttMethod = tcpInfoClass.getMethod("rtt"); | ||
|
|
||
| long totalRetrans = (Long) totalRetransMethod.invoke(info); | ||
| int retransmits = (Integer) retransmitsMethod.invoke(info); |
There was a problem hiding this comment.
Are these cumulative metrics or are these total retransmits since the connection start? If latter, the current logic is incorrect.
2 participants
No description provided.