Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-4345: Run decktape test for each pull request #2064

Closed
wants to merge 1 commit into from

Conversation

raghavgautam
Copy link
Contributor

As of now the ducktape tests that we have for kafka are not run for pull request. We can run these test using travis-ci. Here is a sample run:
https://travis-ci.org/raghavgautam/kafka/builds/170574293

@raghavgautam
Copy link
Contributor Author

Requesting @harshach to review.

@harshach
Copy link

Thanks @raghavgautam . I'll go over it.

VOLUME ["/kfk_src"]

ENV MIRROR="http://apache.cs.utah.edu/"
RUN wget -q "${MIRROR}kafka/0.8.2.2/kafka_2.10-0.8.2.2.tgz" -O "/tmp/kafka_2.10-0.8.2.2.tgz" && tar xfz /tmp/kafka_2.10-0.8.2.2.tgz -C /opt && mv "/opt/kafka_2.10-0.8.2.2" "/opt/kafka-0.8.2.2"
Copy link

@harshach harshach Oct 28, 2016

Choose a reason for hiding this comment

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

if we are using released binaries it means we are not running against the PR right. Shouldn't we merge the PR and build Kafka distro out of it and then run tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We build and copy the new binary to /opt/kafka-trunk and test that. We need released binaries for compatibility tests.

@harshach
Copy link

harshach commented Nov 1, 2016

@raghavgautam does all of the tests pass and do we need to ask for any infra changes for this to run?

@raghavgautam
Copy link
Contributor Author

raghavgautam commented Nov 1, 2016

@harshach
We can run these tests in two way, locally on the laptop and on travis-ci.

For the enabling travis-ci, we need community consensus and I will start a discussion for this.

@raghavgautam
Copy link
Contributor Author

I have disabled failing splits.
https://travis-ci.org/raghavgautam/kafka/builds/174369515

@harshach
Copy link

+1.

@harshach
Copy link

@raghavgautam can you upmerge this patch. I'll merge it in today.

@raghavgautam
Copy link
Contributor Author

@harshach I have rebased my pull request

@asfgit asfgit closed this in e035fc0 Nov 24, 2016
@harshach
Copy link

Thanks @raghavgautam . Merged into trunk. Did you open a infra ticket for this.

@ijuma
Copy link
Contributor

ijuma commented Nov 24, 2016

I think it's great to be able to run the system tests in Travis. However, I don't understand why we added the gradle wrapper? It's the wrong version (we are using Gradle 3.2) and it was an intentional decision to remove it some time ago. Also, the system test renames are a bit arbitrary and they make it harder to use in the normal case, which is a point that Ewen had raised in the mailing list discussion.

@ijuma
Copy link
Contributor

ijuma commented Nov 24, 2016

Cc @ewencp

@ewencp
Copy link
Contributor

ewencp commented Nov 25, 2016

tests/cluster_file.json should also probably be under tests/travis/ since it is highly specific to that configuration. It'd also generally be a good idea to run a test using the existing implementation when there's this much movement under these tests to make sure everything still works (and that there are the expected number of tests -- one of the easiest things to break when moving things around is that some things get lost in directories without proper __init__.py files and become unimportable).

@ewencp
Copy link
Contributor

ewencp commented Nov 25, 2016

Also, trunk doesn't build since we're missing various licenses in files that don't pass the rat check...

:rat
Unknown license: /Users/ewencp/kafka.git/gradle/wrapper/gradle-wrapper.properties
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/client1/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/core1/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/mirror_maker/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/replication/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/security1/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/security2/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/kafkatest/tests/upgrade/__init__.py
Unknown license: /Users/ewencp/kafka.git/tests/travis/ssh/id_rsa
Unknown license: /Users/ewencp/kafka.git/tests/travis/ssh/id_rsa.pub

@raghavgautam
Copy link
Contributor Author

@ewencp I will open a pull request for the rat issue asap and will open jira's for rest of the issues.

@ewencp
Copy link
Contributor

ewencp commented Nov 25, 2016

@raghavgautam Sorry, should have posted here, I filed #2172 already, which also removes the gradle wrapper and gets things setup in Travis via the travis.yml. See discussion there about whether we want to incrementally sort this out or just back out the change and re-file the PR so we can work on it until we've addressed the concerns previously mentioned.

@raghavgautam
Copy link
Contributor Author

@ewencp I too filed https://issues.apache.org/jira/browse/KAFKA-4448 and started working on it.

@ewencp
Copy link
Contributor

ewencp commented Nov 28, 2016

@harshach Thoughts on reverting this given some of the issues raised vs trying to clean things up with follow up commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants