-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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-3592: System test - configurable paths #1245
Conversation
@ewencp ping for initial review There are some rough edges and unfinished bits, but It would be great to get feedback on the overall approach and path resolution api. As we discussed offline recently, it's possible we want to change "path resolution" to something more general. From my vantage point, this is fairly extensible, but ideas here are welcome as well. Note that running tests here relies on logic in the |
091e915
to
08c9cba
Compare
""" | ||
|
||
DEFAULT_SCRATCH_ROOT = "/mnt" | ||
DEFAULT_KAFKA_INSTALL_ROOT = "/opt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these can be overridden, should we remove the DEFAULT_
prefix?
e219d19
to
10b64ad
Compare
@ewencp Mind taking a quick look at the path resolution API again? I think it's cleaner now |
f50b630
to
4d62add
Compare
@ewencp I think this is ready for a more careful review now |
58aede0
to
425b5b0
Compare
import time | ||
from kafkatest.services.zookeeper import ZookeeperService | ||
from kafkatest.utils.remote_account import line_count, file_exists | ||
from kafkatest.version.version import LATEST_0_8_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these imports repeating version
? It looks like that module only contains one child module.
@granders Overall looks nice; easy transition without much overhead at each call. I'm having trouble running unit tests. First, I think we missed a |
Also, I assume we'll want to kick off a full test run before merging this since it's a pretty substantial change? And you might want to check pre- and post- patch |
425b5b0
to
fdb75f3
Compare
@ewencp Regarding unit tests - the kafkatest unit tests don't actually depend on memory_profiler, but the I'll try to fix this |
Running branch builder for further verification: Also - I don't see errors with |
8b3145e
to
bcf3982
Compare
ping @ewencp As far as I can tell digging into logs, these failures aren't related to path resolution changes |
LGTM, thanks! |
This patch adds logic for the following: - remove hard-coded paths to various scripts and jars in kafkatest service classes - provide a mechanism for overriding path resolution logic with a "pluggable" path resolver class Author: Geoff Anderson <geoff@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes #1245 from granders/configurable-install-path (cherry picked from commit 54092c1) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
This patch adds logic for the following: - remove hard-coded paths to various scripts and jars in kafkatest service classes - provide a mechanism for overriding path resolution logic with a "pluggable" path resolver class Author: Geoff Anderson <geoff@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#1245 from granders/configurable-install-path
This patch adds logic for the following: