MINOR: Cleanup redundancies in BaseRequestTest by hachikuji · Pull Request #7735 · apache/kafka

hachikuji · 2019-11-22T02:25:22Z

This patch eliminates a lot of redundancy and general messiness around the usage of BaseRequestTest and specifically response deserialization.

Committer Checklist (excluded from commit message)

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

ijuma

Thanks for the PR. Nice clean-up. A few minor comments.

ijuma · 2019-11-25T15:13:04Z

      socket.setSoTimeout(1000)
      try {
-        val response = sendAndReceive(produceRequest, ApiKeys.PRODUCE, socket)
-        assertEquals(0, response.remaining)


Why don't we need this assertion anymore?

The assertion was actually never getting executed. The call to sendAndReceive raises a timeout exception, which is what verifyMaxConnections is looking for.

ijuma · 2019-11-25T15:16:09Z

+import org.apache.kafka.common.protocol.ApiKeys
+import org.apache.kafka.common.requests.{ApiVersionsRequest, ApiVersionsResponse}
+
+abstract class AbstractApiVersionsRequestTest extends BaseRequestTest {


Is this abstract base class worth it? We could either add this single method to BaseRequestTest or TestUtils.

It's a bit more justifiable if we take the validateApiVersionsResponse method out of the ApiVersionsRequestTest companion.

Maybe we should move this method to the companion? Nice to avoid long inheritance hierarchies.

We could but it's a little more awkward since it requires an instance of BaseRequestTest. We could also add it to BaseRequestTest, but I was hoping to keep that class generic and to avoid leaking a bunch of api-specific noise into it.

ijuma · 2019-11-25T15:18:16Z

  protected def error(error: Errors, errorMessage: Option[String] = None): ApiError =
    new ApiError(error, errorMessage.orNull)

-  protected def toStructWithDuplicateFirstTopic(request: CreateTopicsRequest): Struct = {


Interesting that this is no longer used. Do you know if we are still testing the case where we have a duplicate topic?

I tracked the removal of the uses of this method back to this: e2e8bdb#diff-217dea05d3ff55c8bf10503c329f7c2b. I don't see a clear reason for it.

@cmccabe do you remember why the test was removed?

CreateTopicsRequest was the first RPC converted over to automatic generation, if I remember correctly. I converted it while writing the generator.

I think when the automatic code generation was first developed, it didn't have support for multiple identical elements in the same map. So it wasn't possible to serialize the invalid request at that point. So I had to remove this test. Later on I added support for duplicate entries in the same map.

We should definitely re-add a test for duplicate createTopics elements. There should also be a test like this in KafkaAdminClientTest to test the admin client's handling of it (AC should not allow this to even get to the server)

Can we file a JIRA please?

ijuma · 2019-11-25T15:21:05Z

@@ -65,21 +65,20 @@ class ApiVersionsRequestTest extends BaseRequestTest {
  @Test
  def testApiVersionsRequestValidationV0(): Unit = {
    val apiVersionsRequest = new ApiVersionsRequest.Builder().build( 0.asInstanceOf[Short])


Existing issue, but empty space before 0 should be removed.

ijuma · 2019-11-25T15:23:14Z

+    connectAndReceive[DeleteTopicsResponse](request, destination = socketServer)
  }

  private def sendMetadataRequest(request: MetadataRequest): MetadataResponse = {


Is it even worth having this method? There are a few other examples like that in other classes.

ijuma · 2019-11-25T15:29:08Z

    }
  }

-  private def responseThrottleTime(apiKey: ApiKeys, response: Struct): Int = {


Glad to see this go. I had done something similar in https://github.com/apache/kafka/pull/7409/files#diff-8259941614685619733d9f458762a0b6L564, but hadn't gotten around to pushing that PR over the line.

ijuma · 2019-11-25T15:41:48Z

+                                               apiVersion: Short,
+                                               clientId: String = "client-id",
+                                               correlationIdOpt: Option[Int] = None
+                                              ): RequestHeader = {


Nit: this is a bit unusual, any reason for it not to be on the previous line?

ijuma · 2019-11-25T15:49:04Z

  @Test
  def testApiVersionsRequestBeforeSaslHandshakeRequest(): Unit = {
-    val plaintextSocket = connect(protocol = securityProtocol)
+    val plaintextSocket = connect()


Maybe we should rename this to socket as the current name is a bit misleading. Same for the other methods.

zhengb0 · 2019-12-04T08:34:31Z

I'm sorry, I'm not careful

ijuma

LGTM

hachikuji · 2019-12-04T19:20:21Z

retest this please

hachikuji · 2019-12-04T22:01:52Z

retest this please

hachikuji · 2019-12-05T03:23:57Z

retest this please

…Test (apache#7735) This patch eliminates some redundancy and general messiness around the usage of `BaseRequestTest` and specifically response deserialization. Reviewers: Ismael Juma <ismael@juma.me.uk>

ijuma reviewed Nov 22, 2019

View reviewed changes

Comment thread core/src/test/scala/unit/kafka/server/BaseRequestTest.scala Outdated

hachikuji force-pushed the cleanup-base-request-test branch 2 times, most recently from 902502c to 5063081 Compare November 22, 2019 19:54

hachikuji added 3 commits November 22, 2019 22:09

MINOR: Cleanup redundancies in BaseRequestTest

f6984a1

Use implicit class parameter

9291106

Some moar simplifications

275d34b

hachikuji force-pushed the cleanup-base-request-test branch from 04e6304 to 275d34b Compare November 23, 2019 06:09

Remove unneeded deserialization method

6bfa953

ijuma self-assigned this Nov 24, 2019

ijuma reviewed Nov 25, 2019

View reviewed changes

hachikuji added 3 commits November 25, 2019 10:11

Address a few comments

c68f770

Relocate ApiVersions test helper

e4a029d

Fix compile error in AddPartitionsToTxnRequestTest

153badb

zhengb0 approved these changes Dec 4, 2019

View reviewed changes

ijuma approved these changes Dec 4, 2019

View reviewed changes

hachikuji merged commit eaccb92 into apache:trunk Dec 5, 2019

Conversation

hachikuji commented Nov 22, 2019

Committer Checklist (excluded from commit message)

Uh oh!

Uh oh!

ijuma 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!

hachikuji Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zhengb0 commented Dec 4, 2019

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji commented Dec 4, 2019

Uh oh!

hachikuji commented Dec 4, 2019

Uh oh!

hachikuji commented Dec 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hachikuji Nov 26, 2019 •

edited

Loading