-
Notifications
You must be signed in to change notification settings - Fork 988
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
Remove root project testing jar #1439
Conversation
When executing integration test code, most integrations expect a jar to be provisioned that contains all test code so that it may be loaded by the processing frameworks directly. Historically to support this we have packaged all integration test code into one big itest jar in the root project. This PR removes that shared central jar. Projects now each build and use their own personal itest jar for use in integration tests. These jars may repackage code from other projects itest jars, but they are insulated within each project that makes use of them.
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.
A couple suggestions, the later probably outside the scope of this PR.
LGTM as-is, or can re-review if you want to embark on the later suggested change.
buildSrc/src/main/groovy/org/elasticsearch/hadoop/gradle/BuildPlugin.groovy
Outdated
Show resolved
Hide resolved
@@ -21,6 +21,14 @@ jar { | |||
} | |||
} | |||
|
|||
itestJar { | |||
from(zipTree(project(":elasticsearch-hadoop-mr").jar.archivePath)) { |
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.
I think the preferred way to model this is to via configuration/artifacts/dependencies instead of referencing the project directly.
So ... this build.gradle would looks similar to
configurations {
sharedHadoop
sharedSpi
}
...
itestJar {
from configurations.sharedHadoop
from configurations.sharedSpi
}
...
dependencies {
...
sharedHadoop project(path: ':elasticsearch-hadoop-mr', configuration: 'sharedHadoop')
sharedSpi project(path: ':elasticsearch-hadoop-mr', configuration: 'sharedSpi')
}
and mr/build.gradle would look something like
configurations {
sharedHadoop
sharedSpi
}
artifacts{
sharedHadoop(new File(projectDir, "src/main/java/org/elasticsearch/hadoop"))
sharedSpi(new File(projectDir, "src/main/resources/META-INF/services"))
}
(and don't have an answer for the generated esh-build.properties)
However, implementing something like that is probably outside the scope of this PR especially since what you have here matches the existing model.
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.
This work is already in progress, spread across multiple changes at the moment. This work sort of highlighted the problem. I can combine those PR's together but I would like to keep them split up so they're easier to review.
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.
I agree with Jake's comments on moving toward using project dependencies rather than direct dependencies on cross-project tasks but we should probably address that later. Otherwise, just a couple of comments.
} | ||
if (project != project.rootProject) { | ||
Jar itestJar = project.tasks.create('itestJar', Jar) | ||
itestJar.dependsOn(project.tasks.getByName('jar')) |
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 do we need the regular JAR? We are just packaging the sourceset output, I don't actually need to bundle the normal JAR to create this one.
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.
Hadoop maintains a separate classpath that needs to be configured for each job in the driver program. We use the local job runners in the integration tests, but they still use their own classloaders when running the local job.
Historically, we configured the job jar (which contains the code for the integ test jobs, as well as the project code being tested) once on the test config. I'd like to move to using the original jar as-is without uber-jarring it with the itest code, but that will require updating every test case since it needs to be configured on every test job (can't be set on the configuration once and be done).
buildSrc/src/main/groovy/org/elasticsearch/hadoop/gradle/BuildPlugin.groovy
Outdated
Show resolved
Hide resolved
@@ -42,7 +42,7 @@ | |||
// init ES-Hadoop JAR | |||
// expect the jar under build\libs | |||
try { | |||
File folder = new File(".." + File.separator + "build" + File.separator + "libs" + File.separator).getCanonicalFile(); | |||
File folder = new File("build" + File.separator + "libs" + File.separator).getCanonicalFile(); |
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.
Let's avoid hardcoding "build" and instead use project.buildDir
.
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.
I totally agree, I'll fix this in a follow up.
When executing integration test code, most integrations expect a jar to be provisioned that contains all test code so that it may be loaded by the processing frameworks directly. Historically to support this we have packaged all integration test code into one big itest jar in the root project. This PR removes that shared central jar. Projects now each build and use their own personal itest jar for use in integration tests. These jars may repackage code from other projects itest jars, but they are insulated within each project that makes use of them.
Relates #1423 |
ES-Hadoop currently relies on its integration test code to be jar'd up before the tests are executed since many of the integrations rely on a jar being provided in order to run test code. We have a decent amount of shared code in the integration test projects, so we package it all up into one big jar on the root project, which makes configuring tests very confusing.
This PR removes that root testing jar and instead builds an itest jar in each project that needs it. We still have an issue with the shared code between integration tests, but there will be a follow up PR to address that.