KAFKA-7210: Add a system test to verify the log compaction by omkreddy · Pull Request #5226 · apache/kafka

omkreddy · 2018-06-14T13:45:35Z

Update TestLogCleaning tool to use Java consumer and rename as LogCompactionTester
Enable log compaction in System tests
Remove configs with values same as server defaults from "kafka.properties" template file.
Update the kafka.py logic to handle the duplicates between kafka.properties and server_prop_overides.

omkreddy · 2018-06-14T13:47:30Z

@ijuma Pls take a look. I have added a system test to verify compaction using this tool.
It will be good if we can merge this along with old consumer changes.

omkreddy · 2018-06-14T18:08:30Z

retest this please

ijuma

Thanks @omkreddy, this is very useful. @cmccabe, do you think you could review this?

ijuma · 2018-06-16T04:41:43Z

Why do we have to do this?

"kafka.properties" template file contains default config values to populate server.properties.

Now log cleaner is enabled by default. So I have removed this property to enable it by default is tests.

"log.segment.bytes" property configured with sever default value and not updated by any of the test.

For now I reverted this change. if required we can do this cleanup later.

Also updated the logic in kafka.py to cleanup the duplicates between kafka.properties and server_prop_overides. Updated the logic to load the kafka.properties first and then override any server_prop_overides configs.

I think it's good to enable the cleaner in the system tests. And if the other property is just the default, I agree we should just remove it from here.

cmccabe · 2018-06-16T04:57:09Z

We should a subdirectory of /mnt, not directly inside /mnt

Updated the code

cmccabe · 2018-06-16T04:57:23Z

Can we just use &>> ?

with "&>>" I am unable to run the command as background job. I will try again.

It's fine to leave it as-is if bash is being fiddly.

cmccabe · 2018-06-16T05:00:18Z

This should be named LogCompactionTool. It is not a test (it is a service used by tests)

omkreddy · 2018-06-16T09:09:33Z

@cmccabe Thanks for the review. Addressed the review comments.

Also updated the logic in kafka.py to cleanup the duplicates between kafka.properties and server_prop_overides. We load the kafka.properties first and then override any server_prop_overides configs.

Pls take a look.

ijuma · 2018-06-17T23:18:58Z

This should remain in the test directory right? It's similar to MiniKdc.

It is similar to ReplicaVerificationTool. So i moved to src directory. This will be useful, if we want to test manually with binary distribution.

Also, adding testlibs to minikdc service classpath handled separately. not sure we want do same here.
https://github.com/apache/kafka/blob/trunk/tests/kafkatest/services/security/minikdc.py#L110MiniKdc

A KIP is needed for adding new user visible tools. Personally, I don't see much value in testing the binary distribution manually.

Ok.. moved tool class to test directory and retained the original name.

omkreddy · 2018-06-20T19:15:32Z

@ijuma @cmccabe rebased the PR. Verified most of the system tests on my local machine.
Is it possible to start system test run for this branch?

ijuma · 2018-07-25T06:29:02Z

@cmccabe, can you please review?

cmccabe · 2018-07-25T16:40:02Z

The should be --bootstrap-server to match how our tools work. (Some of them use nonstandard synonyms currently, but we want to converge on --bootstrap-server)

cmccabe · 2018-07-25T16:40:45Z

Can we specify in the help text that this is the time in milliseconds?

cmccabe · 2018-07-25T16:41:54Z

Can we use nextLong here to be more certain we won't get a collision?

cmccabe · 2018-07-25T16:43:51Z

We should have a try/catch block here to prevent the AdminClient object from leaking.

cmccabe · 2018-07-25T16:45:21Z

let's have a try/catch block here as well to prevent a leak.

cmccabe · 2018-07-25T16:46:39Z

Now that we're using Java8, you can just use Files.newBufferedWriter here

cmccabe · 2018-07-25T16:47:17Z

can you add parentheses to make this expression clearer

cmccabe · 2018-07-27T21:51:13Z

LGTM. Thanks, @omkreddy

omkreddy · 2018-08-06T18:38:55Z

@ijuma Can you please take a look?

ijuma

Thanks for the PR, it will be great to have this. I have a few comments.

ijuma · 2018-08-13T02:08:19Z

Can you please remind me why we need to do this again?

I am using TestUtils.waitUntilTrue() method in the test tool, which intern depends on clients/test/TestUtils.java. I added this line include client test library to system test dependent libs,

Why doesn't the following do it?

from (configurations.testRuntime) { include('*.jar') }

It seems like it should include every runtime dependency jar?

current "configurations.testRuntime" does not include dependent project (clients) test jar/dependencies.

Updated build.gradle based on below link, to include dependent project test dependencies.

https://softnoise.wordpress.com/2014/09/07/gradle-sub-project-test-dependencies-in-multi-project-builds/

We can decide whether to have this commit (or) previous one line change.

ijuma · 2018-08-13T02:14:21Z

I'm confused about this message. Shouldn't we explain that arguments are expected or something like that?

ijuma · 2018-08-13T03:55:14Z

Is this used by anything?

this is part of legacy tool which can be used to dump the log contents (Similar dump log tool). We can remove If we think it is not required.

Yeah, if we don't have an immediate use, I'd remove it.

removed the dumpOpt option

ijuma · 2018-08-13T04:00:01Z

Maybe we can use the f interpolator here https://docs.scala-lang.org/overviews/core/string-interpolation.html

ijuma · 2018-08-13T04:02:56Z

It would be helpful to know which topics have not been created.

ijuma · 2018-08-13T04:11:54Z

Nit: a few unnecessary ()

ijuma · 2018-08-13T04:13:15Z

Can we just use Utils.utf8?

There's a version that takes a ByteBuffer, so you can simplify more.

ijuma · 2018-08-13T04:14:34Z

These should probably be parse methods in the companion object (if they are used).

added companion object. not sure, if this is what you're suggesting. Please take a look.

ijuma · 2018-08-13T04:15:29Z

What's the default for this?

default is : 102400, not sure if we can remove this line

I suggest we remove it.

ijuma · 2018-08-13T04:19:01Z

How long does this test usually take? Are we sure that 2 minutes is enough?

For the configured system test inputs (1million small messages) it will take 60seconds to complete.

OK, let's make it 180 seconds then.

omkreddy · 2018-08-13T13:41:48Z

@ijuma Thanks for the review. Updated the review . Pls take one more look.

omkreddy · 2018-08-13T16:28:56Z

retest this please

ijuma

Thanks for the updates.

ijuma · 2018-08-18T17:40:30Z

Seems like you can do the filter here instead of forall and then the message creation below becomes simpler.

ijuma · 2018-08-18T17:41:07Z

Nit: you can remove [Array[Byte], Array[Byte]]

ijuma · 2018-08-18T17:41:37Z

Nit: you can remove [String, String].

ijuma · 2018-08-18T17:42:08Z

There's a version that takes a ByteBuffer, so you can simplify more.

ijuma · 2018-08-18T17:42:44Z

The idea was to call this method parse to make it clear that it's parsing the line and can fail.

ijuma · 2018-08-18T17:43:30Z

Is this used outside of the TestRecord.apply method? If so, I'd move it to the companion object too and give it another name (maybe parseComponents). Otherwise, I'd just inline it.

Updated the code. Pls take a look.

ijuma · 2018-08-18T17:45:57Z

I think I'd say something about the fact that no arguments were passed, but some are expected.

This line used for printing usage. next line checks for required arguments. Same pattern we are using in other tools.

ijuma · 2018-08-18T17:47:36Z

Why doesn't the following do it?

from (configurations.testRuntime) { include('*.jar') }

It seems like it should include every runtime dependency jar?

ijuma · 2018-08-19T16:33:32Z

+  }
+
+  artifacts {
+    testOutput jarTest


Interesting. Does this create a new artifact? I think maybe we should go back to your previous solution that is limited to copyDependantTestLibs with a comment explaining why it's needed. We can consider separately whether we should move away from testCompile project(':clients').sourceSets.test.output in favour of the more complete solution across the board (we use test dependencies in multiple modules).

There's also the option of just using https://plugins.gradle.org/plugin/com.github.hauner.jarTest/1.0, but it's best to discuss that in a separate PR.

makes sense. reverted the last commit. Will handle this in separate PR.
For now, added a comment to explain the need for previous solution.

This reverts commit 02e8edf.

ijuma

Thanks for your patience, LGTM.

This is a fix to apache#5226 to account for config properties that have an equal char in the value. Otherwise if there is one equal char in the value the following error occurs: ``` dictionary update sequence element #XX has length 3; 2 is required ```

) This is a fix to #5226 to account for config properties that have an equal char in the value. Otherwise if there is one equal char in the value the following error occurs: dictionary update sequence element #XX has length 3; 2 is required Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Ismael Juma <ismael@juma.me.uk>

* Updated TestLogCleaning tool to use Java consumer and rename as LogCompactionTester. * Enabled the log cleaner in every system test. * Removed configs from "kafka.properties" with default values and `socket.receive.buffer.bytes` as the override did not seem necessary. * Updated `kafka.py` logic to handle duplicates between `kafka.properties` and `server_prop_overrides`. * Updated Gradle build so that classes from `kafka-clients` test jar can be used in system tests. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Ismael Juma <ismael@juma.me.uk>

…ache#5549) This is a fix to apache#5226 to account for config properties that have an equal char in the value. Otherwise if there is one equal char in the value the following error occurs: dictionary update sequence element #XX has length 3; 2 is required Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Ismael Juma <ismael@juma.me.uk>

) (#7042) This is a fix to #5226 to account for config properties that have an equal char in the value. Otherwise if there is one equal char in the value the following error occurs: dictionary update sequence element #XX has length 3; 2 is required Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Ismael Juma <ismael@juma.me.uk>

omkreddy force-pushed the TestLogClean branch 3 times, most recently from 3fd4735 to ae00322 Compare June 14, 2018 13:56

ijuma reviewed Jun 16, 2018

View reviewed changes

cmccabe reviewed Jun 16, 2018

View reviewed changes

omkreddy force-pushed the TestLogClean branch 2 times, most recently from 8343549 to aa2288a Compare June 16, 2018 08:20

omkreddy force-pushed the TestLogClean branch 2 times, most recently from 744a64f to d73cce2 Compare June 17, 2018 12:00

ijuma reviewed Jun 17, 2018

View reviewed changes

omkreddy force-pushed the TestLogClean branch 2 times, most recently from 59a529c to 46a6c30 Compare June 18, 2018 09:37

omkreddy changed the title ~~MINOR: Update TestLogCleaning tool to use java consumer~~ MINOR: Update TestLogCleaning tool to use Java consumer Jun 18, 2018

omkreddy force-pushed the TestLogClean branch 2 times, most recently from abbb03a to 0f6b938 Compare June 20, 2018 04:30

ijuma added this to the 2.1.0 milestone Jun 27, 2018

cmccabe reviewed Jul 25, 2018

View reviewed changes

ijuma reviewed Aug 13, 2018

View reviewed changes

omkreddy force-pushed the TestLogClean branch from b9fdd02 to 8e496a7 Compare August 13, 2018 13:25

ijuma reviewed Aug 18, 2018

View reviewed changes

omkreddy added 6 commits August 18, 2018 23:40

MINOR: Update TestLogCleaning tool to use java consumer

159593a

Use adminclient to create compacted topics

29c402b

Address colin's comments

3374c2f

Update file names

5464089

Address latest review comments

84c0c95

Address review comments

fdd7281

omkreddy force-pushed the TestLogClean branch from 8e496a7 to fdd7281 Compare August 18, 2018 19:00

omkreddy added 2 commits August 19, 2018 01:37

Remove apply method

c2545b5

Add client test jar/dependencies to core/ configurations

02e8edf

ijuma reviewed Aug 19, 2018

View reviewed changes

omkreddy added 2 commits August 20, 2018 12:59

Revert "Add client test jar/dependencies to core/ configurations"

6f7dff7

This reverts commit 02e8edf.

Add a comment to build.gradle

80332b9

ijuma approved these changes Aug 20, 2018

View reviewed changes

ijuma merged commit 914ffa9 into apache:trunk Aug 20, 2018

omkreddy deleted the TestLogClean branch August 20, 2018 11:26

rayokota mentioned this pull request Aug 21, 2018

MINOR: only split at first occurrence of '=' #5549

Merged

3 tasks

kkonstantine mentioned this pull request Jul 8, 2019

MINOR: Split at first occurrence of '=' in kafka.py props parsing (cherry-pick on 2.0) #7042

Merged

3 tasks

Conversation

omkreddy commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omkreddy commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omkreddy commented Jun 14, 2018

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!

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!

omkreddy commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

omkreddy commented Jun 20, 2018

Uh oh!

ijuma commented Jul 25, 2018

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!

cmccabe commented Jul 27, 2018

Uh oh!

omkreddy commented Aug 6, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkreddy Aug 13, 2018 • 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!

omkreddy Aug 18, 2018 • 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!

omkreddy commented Jun 14, 2018 •

edited

Loading

omkreddy commented Jun 14, 2018 •

edited

Loading

omkreddy commented Jun 16, 2018 •

edited

Loading

omkreddy Aug 13, 2018 •

edited

Loading

omkreddy Aug 18, 2018 •

edited

Loading

omkreddy Aug 13, 2018 •

edited

Loading