MINOR: Added unit tests for ConnectionQuotas by apovzner · Pull Request #8650 · apache/kafka

apovzner · 2020-05-12T04:29:37Z

Added ConnectionQuotasTest, a unit test for ConnectionQuotas. We test connection limits functionality in SocketServerTest (max connections per IP), and DynamicConnectionQuotaTest (max broker-wide and per listener connection limits), which is an integration test. It is useful to have unit tests that directly test ConnectionQuotas.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

dajac

@apovzner Thanks for the PR. Overall, the logic of the tests looks good to me. I have left a bunch of comments/questions.

dajac · 2020-05-12T08:14:39Z

+      executor.submit((() =>
+        intercept[RuntimeException](connectionQuotas.inc(externalListener.listenerName, externalListener.defaultClientIp, blockedPercentMeters("EXTERNAL")))): Runnable
+      ).get(5, TimeUnit.SECONDS)


I think that intercept does not assert that the exception is thrown but return the intercepted exception. It would be better to use assertThrown here.

Do we really need to use an executor here? It seems that inc should throw immediately if the listener does not exist.

intercept also verifies the exception. For example, if the exception thrown is different, the test will fail with an error like this: Expected exception java.lang.IllegalArgumentException to be thrown, but kafka.network.TooManyConnectionsException was thrown. If if no exception is thrown, it will fail like this: Expected exception java.lang.IllegalArgumentException to be thrown, but no exception was thrown. From what I see, assertThrown achieves the same behavior as intercept.

About using the executor -- you are right that inc should return immediately in this case. I still used the executor in case the behavior changes and inc blocks. I think it's better to protect the test from this scenario

dajac · 2020-05-12T08:19:40Z

+    try {
+      // verify there is no limit by accepting 10000 connections as fast as possible
+      val numConnections = 10000
+      val futures = listeners.values.map( listener =>


nit: the space before listener is not needed. i would use curly braces here to stay consistent with the foreach below.

dajac · 2020-05-12T08:20:23Z

+      // verify there is no limit by accepting 10000 connections as fast as possible
+      val numConnections = 10000
+      val futures = listeners.values.map( listener =>
+        executor.submit((() => acceptConnections(connectionQuotas, listener, numConnections)): Runnable) )


nit: the space before the last closing parenthesis is not needed.

dajac · 2020-05-12T08:25:02Z

+        executor.submit((() => acceptConnections(connectionQuotas, listener, numConnections)): Runnable) )
+      futures.foreach(_.get(10, TimeUnit.SECONDS))
+      listeners.values.foreach { listener =>
+        assertEquals(s"${listener.listenerName.value()}",numConnections, connectionQuotas.get(listener.defaultClientIp))


Number of connections on $listener: to stay consistent with the other tests?

nit: a space is missing after the first coma.

dajac · 2020-05-12T08:26:06Z

+        // calling dec() for an IP for which we didn't call inc() should throw an exception
+        intercept[IllegalArgumentException](connectionQuotas.dec(listener.listenerName, unknownHost))


I think that this does not assert as mentioned earlier. Moreover, I think that this deserves its own unit test as it is not really related to the current one.

agreed, moved to a separate test

dajac · 2020-05-12T08:50:35Z

+        executor.submit((() => acceptConnections(connectionQuotas, listener, listenerMaxConnections)): Runnable) )
+      futures.foreach(_.get(5, TimeUnit.SECONDS))
+      listeners.values.foreach { listener =>
+        assertEquals(s"${listener.listenerName.value()}", listenerMaxConnections, connectionQuotas.get(listener.defaultClientIp))


Number of connections on $listener: to stay consistent with other tests?

dajac · 2020-05-12T08:50:59Z

+      val futures2 = listeners.values.map( listener =>
+        executor.submit((() => acceptConnections(connectionQuotas, listener, 1)): Runnable) )


nit: curly braces.

dajac · 2020-05-12T08:51:34Z

+      val futures2 = listeners.values.map( listener =>
+        executor.submit((() => acceptConnections(connectionQuotas, listener, 1)): Runnable) )
+      futures2.foreach { future =>
+        intercept[TimeoutException](future.get(1, TimeUnit.SECONDS))


you may want to assert here.

see my comment that intercept does assert on both wrong exception or no exception, similar to assertThrown

dajac · 2020-05-12T08:53:02Z

+        connectionQuotas.dec(listener.listenerName, listener.defaultClientIp)
+      }
+      // all connections should get added
+      futures.foreach(_.get(5, TimeUnit.SECONDS))


Could we assert the number of connections after this line?

dajac · 2020-05-12T08:55:06Z

+      assertEquals(s"Number of connections on $externalListener:", maxConnectionsPerIp + 2, connectionQuotas.get(externalListener.defaultClientIp))
+
+      // connections on the same listener but from a different IP should be accepted
+      executor.submit((() => acceptConnections(connectionQuotas, externalListener.listenerName, knownHost, maxConnectionsPerIp, 0)): Runnable).get(5, TimeUnit.SECONDS)


A general comment. It would be great if we could break long lines in order to keep them under a reasonable size. I don't know if we have a strict guideline on this but we tend to do it.

apovzner · 2020-05-13T20:41:57Z

@dajac Thanks for your very thorough review. I addressed all your comments.

rajinisivaram · 2020-05-15T21:15:19Z

ok to test

rajinisivaram

@apovzner Thanks for the tests, LGTM

gwenshap · 2020-05-15T22:14:48Z

there was an odd build failure.

retest this please.

apovzner · 2020-05-18T19:06:09Z

It looks like the build couldn't even run tests:

15:15:09 ERROR: Error cloning remote repo 'origin'
...
15:15:18 stderr: fatal: Unable to look up github.com (port 9418) (Name or service not known)

gwenshap · 2020-05-19T22:48:20Z

ok to test

mjsax · 2020-05-20T00:02:48Z

Retest this please.

apovzner · 2020-05-20T20:17:28Z

One of the build failures failed due to one of the unit tests added in this PR. It was bug in the test, that was waiting on the wrong future, which had a name similar to another one. I fixed both waiting on the right future and changed variable name so that it is easier to spot.
@mjsax Could you please rerun the tests?

mjsax · 2020-05-21T00:15:23Z

Retest this please

mjsax · 2020-05-21T00:15:40Z

Retest this please.

apovzner · 2020-05-21T03:12:16Z

JDK 14 build has an unrelated test failure: org.apache.kafka.streams.integration.EosBetaUpgradeIntegrationTest.shouldUpgradeFromEosAlphaToEosBeta[false]

It looks like tests passed on other builds, but it failed to record the results.

rajinisivaram · 2020-05-21T16:19:54Z

Test failures unrelated, merging to trunk.

* 'trunk' of github.com:apache/kafka: KAFKA-9980: Fix bug where alterClientQuotas could not set default client quotas (apache#8658) KAFKA-9780: Deprecate commit records without record metadata (apache#8379) MINOR: Deploy VerifiableClient in constructor to avoid test timeouts (apache#8651) MINOR: Added unit tests for ConnectionQuotas (apache#8650) MINOR: Correct MirrorMaker2 integration test configs for Connect internal topics (apache#8653) KAFKA-9855 - return cached Structs for Schemas with no fields (apache#8472) KAFKA-9950: Construct new ConfigDef for MirrorTaskConfig before defining new properties (apache#8608) KAFKA-8869: Remove task configs for deleted connectors from config snapshot (apache#8444) KAFKA-9409: Supplement immutability of ClusterConfigState class in Connect (apache#7942)

MINOR: Added unit test for ConnectionQuotas

9252ff9

dajac reviewed May 12, 2020

View reviewed changes

addressed review comments

117200f

rajinisivaram approved these changes May 15, 2020

View reviewed changes

apovzner added 2 commits May 20, 2020 13:07

fixed test that was waiting on wrong future

df9fd8b

changed variable name in test so it is easier to differentiate

ff2e473

rajinisivaram merged commit f6781f4 into apache:trunk May 21, 2020

		// calling dec() for an IP for which we didn't call inc() should throw an exception
		intercept[IllegalArgumentException](connectionQuotas.dec(listener.listenerName, unknownHost))

		val futures2 = listeners.values.map( listener =>
		executor.submit((() => acceptConnections(connectionQuotas, listener, 1)): Runnable) )

Conversation

apovzner commented May 12, 2020

Committer Checklist (excluded from commit message)

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apovzner commented May 13, 2020

Uh oh!

rajinisivaram commented May 15, 2020

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

gwenshap commented May 15, 2020

Uh oh!

apovzner commented May 18, 2020

Uh oh!

gwenshap commented May 19, 2020

Uh oh!

mjsax commented May 20, 2020

Uh oh!

apovzner commented May 20, 2020

Uh oh!

mjsax commented May 21, 2020

Uh oh!

mjsax commented May 21, 2020

Uh oh!

apovzner commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajinisivaram commented May 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

apovzner commented May 21, 2020 •

edited

Loading