Grant Henke has posted comments on this change. Change subject: KUDU-2066. Add experimental Gradle build support ......................................................................
Patch Set 14: (46 comments) http://gerrit.cloudera.org:8080/#/c/7258/14/build-support/release/rat_exclude_files.txt File build-support/release/rat_exclude_files.txt: Line 14: java/gradlew.bat > Isn't this deleted by wrapper.gradle? Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/.gitignore File java/.gitignore: Line 31: bin > Hmm, what does gradle put in here? I thought all the build output was isola Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/build.gradle File java/build.gradle: Line 30: // they are used to ensure there are no dependency issues > It'd also be good to note that individual subprojects may include other gra Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/artifacts.gradle File java/gradle/artifacts.gradle: Line 18: task testJar(type: Jar, dependsOn: testClasses, group: "Build") { > Why doesn't this one need extension "jar"? Done Line 21: from sourceSets.test.output > Nit: to be consistent with the other definitions, put this before classifie Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/buildscript.gradle File java/gradle/buildscript.gradle: Line 18: repositories { > If any of these are used for just 1-2 plugins, it'd be good to precede thei Done Line 26: // Manage plugin dependencies since the plugin block can't be used in included build scripts yet. > Might be good to link to the online docs that can contextualize "yet". Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/compile.gradle File java/gradle/compile.gradle: Line 18: // Set compilation settings for the project > Can you beef this up a little? For one, this isn't just one "project", righ Done Line 19: allprojects { > Why allprojects and not subprojects? This actually isn't required since we apply it in the subprojects block in the main build. I copied this script from an older project of mine and will simplify it a bit. Line 24: options.fork = true // enable compilation in a separate daemon process. > Hmm, why do this? Done Line 31: } > This doesn't seem specific to Gradle; should it be ported to the Maven buil I am not sure I know how to do this in the maven build. Mainly the ability to detect jav 8 vs java7 Line 38: scalaCompileOptions.additionalParameters = ["-Xlint", "-feature"] // turn on compiler warnings for potential mistakes > Maybe this belongs in Maven too? Happy to do this in a separate patch. Its only command line build output changes, so not critical. http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/dependencies.gradle File java/gradle/dependencies.gradle: Line 28: commonsIo : "2.5", > Nit: not alphabetically sorted. Check the rest too. Done Line 64: def spark2Version = '2.1.1' > Nit: these are only used once on L65-68; may be clearer to just define them I thought this made the below logic more explicit and readable. Line 81: flumeConfiguration: "org.apache.flume:flume-ng-configuration:$versions.flume", > Not alphabetically sorted. Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/properties.gradle File java/gradle/properties.gradle: Line 18: // A Common method to handle loading gradle properties with a default when > Nit: should be lower-case unless it's referring to something with the prope Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/publishing.gradle File java/gradle/publishing.gradle: Line 26: def mavenUrl = project.hasProperty('mavenUrl') ? project.mavenUrl : '' > I thought by defining hasProperty inside ext, you wouldn't need to prefix i We actually defined propertyExists since propertyExists.hasProperty doesn't check system properties. I will switch to using that. Line 35: // To test locally, replace mavenUrl in gradle.properties to file://localhost/tmp/myRepo/ > Would it be useful to use propertyWithDefault() so that mavenUrl could be s Yeah, I will do that. I missed converting the file once they were added. http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/quality.gradle File java/gradle/quality.gradle: Line 20: project.apply plugin: 'pmd' > I think it'd be nice to add a short one-line description explaining what ea Done Line 21: apply plugin: "com.github.ben-manes.versions" > Add a comment explaining what this does, since it's not obvious from the na Done Line 32: // Create an aggregate checkstyle task > I don't really understand what these "aggregate" tasks do. It seems all the Done Line 65: // Configure the versions plugin to only show updates for released versions. > Not understanding this; in what context does the build "show updates" for a This is configuring the dependencyUpdates plugin from the versions plugin. It has nothing to do with checkstyle/findbugs/pmd, but it does have to do with code quality imho. I think staying up to date on project dependencies is important. This task will tell you if there are newer dependencies available for your project. Try running `gradle dependencyUpdates` for an example. http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/scopes.gradle File java/gradle/scopes.gradle: Line 19: apply plugin: 'propdeps' > Nit: sort alphabetically. Or does this order matter? Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/shadow.gradle File java/gradle/shadow.gradle: Line 32: } > I expected to see this in artifacts.gradle, since that's where artifacts {. This is here since only modules that use shading would have this artifact. This is just replacing the default jar from artifacts as commented below. Line 45: project.conf2ScopeMappings.addMapping( > What does this blurb mean? I will add a comment. Line 68: // Handle install and deploy the same. > Nit: "in the same way" (when I see "the same" it's usually referring to a p Done Line 79: // Remove the shaded Dependencies from the pom > Nit: either "shadedDependencies" (to refer directly to the variable referen Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/tests.gradle File java/gradle/tests.gradle: Line 18: // Support parallel unit test execution > Nit: for comments written as sentences in English, terminate with a period. Done Line 20: maxParallelForks = project.hasProperty('maxParallelForks') ? maxParallelForks.toInteger() : 1 > Not propertyWithDefault here? Done Line 27: exceptionFormat = 'full' > Nit: it's a little jarring to see both double quotes and single quotes used Done Line 31: // Adds pattern based integration test support. > I imagine this was done so the Gradle build feels more like the Maven build I do think it is desirable. This allows "fast" unit tests to be run first and then slower integration tests to be run next. Running `gradle check` will run all tests and quality checks. Line 78: check.dependsOn(integrationTest) > What is 'check'? This is the default gradle task for all project validations/checks. In the case of this project it runs all tests and all quality checks. http://gerrit.cloudera.org:8080/#/c/7258/14/java/gradle/wrapper.gradle File java/gradle/wrapper.gradle: Line 28: doLast { > Why is this necessary? Otherwise this code is run during project configuration and not when the task itself is run. Line 29: def wrapperJarPath = "\$APP_HOME/gradle/wrapper/gradle-wrapper.jar" > What is $APP_HOME and where is it defined? It's defined in the wrapper script (gradlew) that we are injecting this into. Line 61: wrapper.finalizedBy removeWindowScript > What does it mean to have two finalizedBy statements? Are they additive or They are additive. http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-client-tools/build.gradle File java/kudu-client-tools/build.gradle: Line 31: testCompile project(path: ":kudu-client", configuration: 'shadowTest') > A little surprised that we don't need "compile project(":kudu-client")" too It's pulled in via kudu-mapreduce transitively. Running the following on this module shows the dependency tree: `gradle dependencies --configuration=compile` http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-client/build.gradle File java/kudu-client/build.gradle: Line 19: import org.gradle.api.internal.artifacts.publish.ArchivePublishArtifact > Not used? Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-flume-sink/build.gradle File java/kudu-flume-sink/build.gradle: Line 18: apply plugin: "com.commercehub.gradle.plugin.avro" > What does this plugin do? It's not for "provided libs.avro"; that just refe It runs the code generation from the schema files. Will add a comment. http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-flume-sink/pom.xml File java/kudu-flume-sink/pom.xml: Line 24: <avro.version>1.8.2</avro.version> > Why did you need to make this change as part of the Gradle build patch? I don't need to do it as a part of this patch. I can do it in a different patch. http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-jepsen/build.gradle File java/kudu-jepsen/build.gradle: Line 34: compile project(":kudu-client") > Shouldn't this depend on the shadow JAR? Done Line 35: compile project(":kudu-client").sourceSets.test.output > Is this a dependency on kudu-client's test JAR? If so, is there any way we Done http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-jepsen/pom.xml File java/kudu-jepsen/pom.xml: Line 34: <jepsen.version>0.1.5</jepsen.version> > Why does this need to be bumped as part of this patch? It doesn't I can make it a separate patch. http://gerrit.cloudera.org:8080/#/c/7258/14/java/kudu-mapreduce/build.gradle File java/kudu-mapreduce/build.gradle: Line 17: > Why don't we need shadow.gradle here? It could be a bug, but in the maven build we don't shade these jars. Happy to add it to both if you think we should be shading them. Line 23: } > This pattern repeats itself quite often; can we define it somewhere once? There isn't a super clean way to do this. What I did instead was create a pull request against async so that we can remove this all together on the next version update: https://github.com/OpenTSDB/async/pull/8 http://gerrit.cloudera.org:8080/#/c/7258/14/java/pom.xml File java/pom.xml: Line 80: <scala-2.11.version>2.11.11</scala-2.11.version> > Why does this need to change? It doesn't need to be in this patch. I can update it in a different patch. http://gerrit.cloudera.org:8080/#/c/7258/14/java/settings.gradle File java/settings.gradle: Line 19: include "kudu-client" > Nit: alphabetical sorting Done -- To view, visit http://gerrit.cloudera.org:8080/7258 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib257cdd019a1f383c886b9238bb47d96576c4421 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
