MINOR: Streams system test fixes/updates by vvcephei · Pull Request #4689 · apache/kafka

vvcephei · 2018-03-12T18:22:50Z

Some changes required to get the Streams system tests working via Docker

To test:

TC_PATHS="tests/kafkatest/tests/streams" bash tests/docker/run_tests.sh

That command will take about 3.5 hours, and should pass. Note there are a couple of ignored tests.

Committer Checklist (excluded from commit message)

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

vvcephei · 2018-03-12T18:41:24Z

So, this wasn't valid python3... It's also the only super invocation in kafkatest that's formatted this way. I'm not sure if it's valid python2.

But it only affects cleanup for the Eos test, and it just generates a warning instead of failing the test. So I can see how this could go unnoticed if the tests are usually run from a clean state instead of cleaning up after themselves.

vvcephei · 2018-03-12T18:43:34Z

This was not valid python3. Maybe everyone else ran setup.py in python2 and haven't run it again since?

note that it's also valid python2 to use parens here, so I'd advocate for keeping this change.

vvcephei · 2018-03-12T18:44:38Z

setup.py generates these directories. I assume we don't want them checked in.

vvcephei · 2018-03-12T18:45:29Z

These test deps were missing. I guess no one has had to run the upgrade/downgrade test in docker. (Jenkins still uses Vagrant)

mjsax · 2018-03-12T20:00:51Z

@vvcephei Can you trigger the branch builder to see if this changes work there? Thx.

guozhangwang · 2018-03-12T22:48:54Z

I think the chmod is done in vagrant/base.sh?

yeah, it is. That's what I found that ultimately tipped me off to what was wrong with the docker code. (it starts at line 104 there)

vvcephei · 2018-03-13T18:46:17Z

Having this in the output reduced my debug cycle time a lot. I've stripped out all my other debugging output. What do you think about leaving this in?

vvcephei · 2018-03-13T18:49:01Z

I got a lot of value out of having this in the output. What do you think about leaving this in?

vvcephei · 2018-03-13T18:51:49Z

Note: I had to drop 0.10.1 because StreamsSmokeTest had a different package name in that version (was: org.apache.kafka.streams.smoketest.StreamsSmokeTest.

I can put it back in, but it would require a conditional in the streams.py::StreamsSmokeTestBaseService. Or we can leave it out.

Thoughts?

I would say leave it out, I believe the change from 0.10.1 to a higher version requires a full stop and can't be done from a rolling restart.

Actually from 0.10.0 to 0.10.1 requires a full stop, from 0.10.1 to newer version does not. But I think it is still fine to leave it out.

vvcephei · 2018-03-13T18:56:33Z

The most substantial change here is the switch from @parameterize to @matrix to ensure we cover every combination of versions.

Downside: this increases the test run time by at least 1 hour!

We can:

accept that
prune the matrix (select just a handful of scenarios like before)
fork multiple jenkins builds of different parts of that matrix to run them in parallel

Thoughts?

vvcephei · 2018-03-13T18:57:48Z

@guozhangwang @bbejeck @mjsax Please take another look at this. It passes on my machine (using docker). I'm running an all-streams jenkins test to make sure it still passes using vagrant and also to measure the new run time.

bbejeck · 2018-03-13T21:01:08Z

I think you can do the zookeeper start in the setUp method

hm. actually, there isn't a setUp method... Are you suggesting I make one?

FWIW, I think it's actually fine like this. The test is linearly readable this way.

There's a setUp method on the Test class that we can override. It's fine like it is, but most of the other tests do ZK start-up in the setUp method, so I was aiming for consistency.

aha, I gotcha. sure, I can do that.

Ok, I'm glad that I know what you were talking about now. I actually think in this case, it's a little better as is, since we want to start the broker with different versions during the test.

We could still do it, but then we'd have to stop the broker and upgrade it and restart it during the test, or only start zk in startUp and start the broker during the tests... I think in this case, it's a more complicated control flow in the tests in exchange for diminishing returns.

I can totally get it for tests that just need zk and kafka running and don't care about the version, but in this case, I think I like it better flat.

bbejeck · 2018-03-13T21:01:41Z

same as above

guozhangwang

Just a note that @bbejeck is working on https://github.com/apache/kafka/pull/4690/files which may have some overlap on the class files, some we would need some rebasing either one way or the other.

guozhangwang · 2018-03-14T00:03:45Z

guozhangwang · 2018-03-14T00:15:39Z

Actually from 0.10.0 to 0.10.1 requires a full stop, from 0.10.1 to newer version does not. But I think it is still fine to leave it out.

guozhangwang · 2018-03-14T00:16:01Z

Remove LATEST_0_10_1 as you mentioned below?

guozhangwang · 2018-03-14T00:17:09Z

vvcephei · 2018-03-14T00:25:55Z

@guozhangwang Thanks. Yeah, I'm prepared to rebase on #4690 once it's merged.

I'm running a full streams system test again now, just to be double-sure that it passes before recommending a merge.

* add 1.0.0 and 1.0.1 Closes #4689

vvcephei · 2018-03-15T15:36:02Z

I've just added 1.0.1, since it's in the mirror now.
I also rebased onto trunk.
I'm running the sys tests once more to be sure they work with 1.0.1, but otherwise, this will be ready to merge.

vvcephei · 2018-03-15T19:05:45Z

Alright, @mjsax @guozhangwang, I think this thing is ready to merge. I re-ran the streams system tests (both locally:docker and in jenkins:vagrant), and they actually all passed this time!

guozhangwang

LGTM!

vvcephei commented Mar 12, 2018

View reviewed changes

vvcephei changed the title ~~WIP: MINOR: System tests~~ MINOR: System tests Mar 12, 2018

guozhangwang reviewed Mar 12, 2018

View reviewed changes

guozhangwang added the streams label Mar 12, 2018

vvcephei changed the title ~~MINOR: System tests~~ WIP (do not merge): Streams system test updates Mar 13, 2018

vvcephei commented Mar 13, 2018

View reviewed changes

vvcephei changed the title ~~WIP (do not merge): Streams system test updates~~ MINOR: Streams system test fixes/updates Mar 13, 2018

bbejeck reviewed Mar 13, 2018

View reviewed changes

Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated

Copy link
Copy Markdown

Member

bbejeck Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

vvcephei mentioned this pull request Mar 13, 2018

MINOR: Rolling bounce upgrade fixed broker system test #4690

Merged

3 tasks

guozhangwang reviewed Mar 14, 2018

View reviewed changes

MINOR: Streams system tests fixes/updates

31719e4

* add 1.0.0 and 1.0.1 Closes #4689

guozhangwang approved these changes Mar 15, 2018

View reviewed changes

guozhangwang merged commit 7006d0f into apache:trunk Mar 15, 2018

vvcephei deleted the system-tests branch March 15, 2018 21:58

Conversation

vvcephei commented Mar 12, 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!

vvcephei Mar 12, 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!

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!

mjsax commented Mar 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei commented Mar 13, 2018

Uh oh!

vvcephei commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck Mar 14, 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!

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

vvcephei commented Mar 14, 2018

Uh oh!

vvcephei commented Mar 15, 2018

Uh oh!

vvcephei commented Mar 15, 2018

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

vvcephei commented Mar 12, 2018 •

edited

Loading

vvcephei Mar 12, 2018 •

edited

Loading

bbejeck Mar 14, 2018 •

edited

Loading