KAFKA-6809: Count inbound connections in the connection-creation metric #5301
Conversation
Previously, the connection-creation metric only accounted for opened connections from the broker.
There was a problem hiding this comment.
@stanislavkozlovski Thanks for the PR. Looks good, left a few minor comments.
| for (int i = 0; i < conns; i++) | ||
| connect(Integer.toString(i), addr); | ||
|
|
||
| selector.poll(0L); |
There was a problem hiding this comment.
We cannot guarantee that this poll will see all completed connections, so it would be better to poll in a loop until the total connections returned fromselector.connected() after the poll equals conns.
| for (int i = 0; i < conns; i++) { | ||
| Thread sender = createSender(serverAddress, randomPayload(1)); | ||
| sender.start(); | ||
| sender.join(1000); |
There was a problem hiding this comment.
Why do we need the join?
There was a problem hiding this comment.
I did not realize that ss.accept() was blocking
|
|
||
| selector.poll(0L); | ||
|
|
||
| assertEquals(getMetric("connection-creation-total").metricValue(), (double) conns); |
There was a problem hiding this comment.
The parameters should be the other way round (expected is conns and actual is the metric value).
| } | ||
| } | ||
|
|
||
| assertEquals(getMetric("connection-creation-total").metricValue(), (double) conns); |
There was a problem hiding this comment.
Same as before, parameters of assertEquals should be the other way round.
| private KafkaMetric getMetric(String name) throws Exception { | ||
| for (Map.Entry<MetricName, KafkaMetric> entry : metrics.metrics().entrySet()) { | ||
| if (entry.getKey().name().equals(name)) | ||
| return entry.getValue(); |
There was a problem hiding this comment.
Now that we can use Java 8 APIs, you can use entrySet().stream().filter(...) to avoid the for loop.
| SocketChannel channel = ss.accept(); | ||
| channel.configureBlocking(false); | ||
|
|
||
| selector.register(Integer.toString(i), channel); |
There was a problem hiding this comment.
Not related to your change, but perhaps we could also verify connection-count metric? This is a gauge, and should also equal conns in both the tests after the connections are established. What do you think?
There was a problem hiding this comment.
Doesn't hurt to guarantee consistency with both metrics imo, so yeah, good suggestion
stanislavkozlovski
changed the title
|
Addressed all the comments, could you take a second look @rajinisivaram ? |
There was a problem hiding this comment.
@stanislavkozlovski Thanks for the update, left a couple more minor comments.
| // Poll continuously, as we cannot guarantee that the first call will see all connections | ||
| for (int i = 0; i < 10; i++) { | ||
| selector.poll(0L); | ||
| if (selector.connected().size() == conns) |
There was a problem hiding this comment.
selector.connected() is cleared after each poll(). So we should keep track of the total number and compare the total against conns.
There was a problem hiding this comment.
Absolutely true, good catch!
| if (selector.connected().size() == conns) | ||
| break; | ||
|
|
||
| Thread.sleep(100); |
There was a problem hiding this comment.
You could just do selector.poll(100) instead of poll(0) + sleep(100). poll() returns when an operation is ready, so we are not waiting unnecessarily.
There was a problem hiding this comment.
@stanislavkozlovski Thanks for the updates, LGTM. Will merge after the PR builds complete.
|
@stanislavkozlovski Thanks for the PR, merging to trunk and 2.0. |
…ic (#5301) Previously, the connection-creation metric only accounted for opened connections from the broker. This change extends it to account for received connections.
…ic (apache#5301) Previously, the connection-creation metric only accounted for opened connections from the broker. This change extends it to account for received connections.
Previously, the connection-creation metric only accounted for opened connections from the broker. This change extends it to account for received connections.
Committer Checklist (excluded from commit message)