MINOR: Pass a streams config to replace the single state dir by guozhangwang · Pull Request #4714 · apache/kafka

guozhangwang · 2018-03-14T22:30:09Z

This is a general change and is re-requisite to allow streams benchmark test with different streams tests. For the streams benchmark itself I will have a separate PR for switching configs. Details:

Create a "streams.properties" file under PERSISTENT_ROOT before all the streams test. For now it will only contain a single config of state.dir pointing to PERSISTENT_ROOT.
For all the system test related code, replace the main function parameter of state.dir with propsFilename, then inside the function load the props from the file and apply overrides if necessary.
Minor fixes.

Committer Checklist (excluded from commit message)

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

guozhangwang · 2018-03-14T22:32:57Z

@mjsax @bbejeck @vvcephei @dguy for reviews.

bbejeck · 2018-03-14T23:17:10Z

@guozhangwang failures are related, unused import

guozhangwang · 2018-03-14T23:20:13Z

Yup, I'm fixing.

guozhangwang · 2018-03-15T21:45:31Z

Triggered system tests: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1471 Succeeded.

bbejeck · 2018-03-16T13:42:35Z

-        stateDir.mkdir();
-
-        final Properties streamsProperties = new Properties();
+        final Properties streamsProperties = Utils.loadProps(propFileName);


propFileName could be null, do a null check here?

I will consider adding the check within the function.

bbejeck · 2018-03-16T13:44:16Z

        final Serde<String> stringSerde = Serdes.String();

-        final Properties streamsProperties = new Properties();
+        final Properties streamsProperties = Utils.loadProps(propFileName);


same here, check for null propFileName

bbejeck · 2018-03-16T13:45:48Z

+        final String propFileName = args.length > 1 ? args[1] : null;
+        final String command = args.length > 2 ? args[2] : null;
+
+        final Properties streamsProperties = Utils.loadProps(propFileName);


bbejeck · 2018-03-16T13:46:14Z


-        final Properties streamsProperties = new Properties();
-        streamsProperties.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, kafka);
+        final Properties streamsProperties = Utils.loadProps(propFileName);


bbejeck · 2018-03-16T13:50:25Z

Thanks @guozhangwang left a few minor comments, otherwise LGTM.

One general question and sorry if I missed something, but where do we supply our own properties file or edit the existing one to add additional settings?

guozhangwang · 2018-03-16T18:56:58Z

One general question and sorry if I missed something, but where do we supply our own properties file or edit the existing one to add additional settings?

Thanks for the comments. For your question, the file is added here:

https://github.com/apache/kafka/pull/4714/files#diff-1dcacbd6daa49466dc7cb89022929dfbR143

that will only encode the state dir for now. In the follow-up PR I will modify simple benchmark python code to allow for other config overridden and remove the hard-coded ones in the java code. Does that make sense?

…se-property-file-for-system-tests

mjsax

Thanks for the PR. Looks good overall. Couple of minor comments.

Can you also run streams system test before we merge this. Thx.

mjsax · 2018-03-16T22:10:57Z

-            props.load(propStream);
+
+        if (filename != null) {
+            try (InputStream propStream = new FileInputStream(filename)) {


Should we log something for this case?

mjsax · 2018-03-16T22:11:45Z

+
+        final Properties props = Utils.loadProps(propFileName);
+
+        String stateDirStr;


nit: add final

mjsax · 2018-03-16T22:16:16Z

        benchmark.run();
    }

    public Properties setStreamProperties(final String applicationId) {


should we change the return type to void?

mjsax · 2018-03-16T22:17:53Z

    private boolean uncaughtException = false;

-    public SmokeTestClient(File stateDir, String kafka) {
+    public SmokeTestClient(Properties streamsProperties, String kafka) {


nit: add final to cleanup code

mjsax · 2018-03-16T22:18:08Z


-    private static KafkaStreams createKafkaStreams(File stateDir, String kafka) {
-        final Properties props = new Properties();
+    private static KafkaStreams createKafkaStreams(Properties props, String kafka) {


nit: add final

mjsax · 2018-03-16T22:20:53Z

        System.out.println("numThreads=" + numThreads);

-        SimpleBenchmark benchmark = new SimpleBenchmark(stateDir, kafka, loadPhase, testName, numRecords, numThreads);
+        SimpleBenchmark benchmark = new SimpleBenchmark(props, kafka, loadPhase, testName, numRecords, numThreads);


should we put kafka and numThread directly into props and reduce number of parameters here (would also simplify all the "passing through" code.

Yes, in my follow-up PR I've done that, just for its scope I tend to keep its changes more focused.

mjsax · 2018-03-16T22:23:02Z

-        String kafka = args[0];
-        String stateDir = args.length > 1 ? args[1] : null;
-        String command = args.length > 2 ? args[2] : null;
+    public static void main(String[] args) throws InterruptedException, IOException {


nit: add final

mjsax · 2018-03-16T22:23:16Z

 public class StreamsStandByReplicaTest {

-    public static void main(String[] args) {
+    public static void main(String[] args) throws IOException {


nit: add final

guozhangwang · 2018-03-17T04:12:00Z

Can you also run streams system test before we merge this. Thx.

Yeah I have triggered a couple streams system test, the first successful one is https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1471

guozhangwang · 2018-03-17T04:27:04Z

Retriggered https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1480/

mjsax

LGTM.

Is there already a follow up PR that address the pending comments?

guozhangwang · 2018-03-19T19:05:19Z

I'll create the follow-up PR once I rebased this one, there are a sequence of them on my local repo :)

pass a streams config to replace the single state dir

5e178b3

mjsax changed the title ~~MIOR: pass a streams config to replace the single state dir~~ MINOR: pass a streams config to replace the single state dir Mar 14, 2018

mjsax added the streams label Mar 14, 2018

mjsax requested review from dguy and mjsax March 14, 2018 22:36

guozhangwang added 2 commits March 14, 2018 16:17

add missing imports

fe5eb9d

minor fixes

aafec4a

guozhangwang added 4 commits March 14, 2018 17:18

try to make python happy

8a273d1

minor fix

4443075

minor fix

19f81a6

fix runtime class def exception

6a4ae8e

guozhangwang changed the title ~~MINOR: pass a streams config to replace the single state dir~~ MINOR: Pass a streams config to replace the single state dir Mar 15, 2018

guozhangwang added 2 commits March 15, 2018 14:39

remove unused imports

fe03278

rebase from trunk

3a33a08

bbejeck reviewed Mar 16, 2018

View reviewed changes

guozhangwang added 2 commits March 16, 2018 11:58

Merge branch 'trunk' of https://github.com/apache/kafka into KMinor-u…

7e296e7

…se-property-file-for-system-tests

address github comments

2e6d3af

mjsax reviewed Mar 16, 2018

View reviewed changes

github commens two

f6fdfec

mjsax approved these changes Mar 19, 2018

View reviewed changes

guozhangwang merged commit 0f364cd into apache:trunk Mar 19, 2018

bbejeck mentioned this pull request Mar 22, 2018

KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 #4746

Merged

guozhangwang deleted the KMinor-use-property-file-for-system-tests branch April 24, 2020 23:49


		final Properties props = Utils.loadProps(propFileName);

		String stateDirStr;

Conversation

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

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

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

Uh oh!

Uh oh!

bbejeck commented Mar 14, 2018

Uh oh!

guozhangwang commented Mar 14, 2018

Uh oh!

guozhangwang commented Mar 15, 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!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Mar 16, 2018

Uh oh!

guozhangwang commented Mar 16, 2018

Uh oh!

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

guozhangwang commented Mar 17, 2018

Uh oh!

guozhangwang commented Mar 17, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guozhangwang commented Mar 14, 2018 •

edited

Loading

guozhangwang commented Mar 14, 2018 •

edited

Loading

guozhangwang commented Mar 15, 2018 •

edited

Loading