You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/07/13 12:38:34 UTC
[GitHub] [iceberg] Rolly992 opened a new pull request #2816: Gradle build cache optimization
Rolly992 opened a new pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816
Update Gradle Wrapper version to 5.6.4
Update ShadowJar plugin to support Gradle Build Cache
Tune compileJava tasks to support Gradle Build Cache via relative path.
Note: Only task which is not cacheable is ScalaStyleTask. This is due to task input paths being absolute and this is not modifiable outside of plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] runningcode commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
runningcode commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r670348281
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
This is not the heap size used for the build and Gradle Daemon. My understanding is that these are the settings used for the Gradle client. See more info on configuring the heap size here: https://docs.gradle.org/current/userguide/build_environment.html#sec:configuring_jvm_memory
The iceberg project has the heap size already configured in the `gradle.properties` here: https://github.com/apache/iceberg/blob/master/gradle.properties#L19
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] snazy commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
snazy commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r668962451
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -21,4 +21,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-5.4.1-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.4-bin.zip
Review comment:
Out of curiosity, have you tried newer Gradle versions?
Newer shadow-plugin versions (which require at least Gradle 6), are noticeable faster - hence the question.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Rolly992 commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
Rolly992 commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r668963706
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
Yes, this the the default generated by gradle wrapper version update.
I decided to leave it based on idea that Gradle knows better how to update itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] snazy commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
snazy commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r668958954
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
That small heap size is fine for Gradle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] runningcode commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
runningcode commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r670346771
##########
File path: gradlew
##########
@@ -64,10 +64,6 @@ case "`uname`" in
;;
esac
-if [ ! -e $APP_HOME/gradle/wrapper/gradle-wrapper.jar ]; then
- curl -o $APP_HOME/gradle/wrapper/gradle-wrapper.jar https://raw.githubusercontent.com/gradle/gradle/v5.4.1/gradle/wrapper/gradle-wrapper.jar
-fi
-
Review comment:
The standard setup for a Gradle build is to include the Gradle wrapper jar in the project. I just noticed that this is not the case for Apache Iceberg. Can we check in the wrapper to make it a more standard setup?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Rolly992 commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
Rolly992 commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r672344446
##########
File path: gradlew
##########
@@ -64,10 +64,6 @@ case "`uname`" in
;;
esac
-if [ ! -e $APP_HOME/gradle/wrapper/gradle-wrapper.jar ]; then
- curl -o $APP_HOME/gradle/wrapper/gradle-wrapper.jar https://raw.githubusercontent.com/gradle/gradle/v5.4.1/gradle/wrapper/gradle-wrapper.jar
-fi
-
Review comment:
I think I need to append this PR with the Gradle wrapper jar in another commit. Will do it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r670166317
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
If you’re optimizing for build cache (and for gradle runtimes in general), I would think raising this might be beneficial. Particularly, for compiling scala code I usually see it recommended that the gradle JVM get more memory.
I would personally suggest `-Xmx1024m -XX:MaxPermSize=256m`, but I also see many articles recommending at least `DEFAULT_JVM_OPTS="-Xmx512m"`.
Your call I guess, but I’d consider being sure that the values you’re using there as the default are still being used currently despite being explicitly zero’d out currently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r670161636
##########
File path: gradlew
##########
@@ -64,10 +64,6 @@ case "`uname`" in
;;
esac
-if [ ! -e $APP_HOME/gradle/wrapper/gradle-wrapper.jar ]; then
- curl -o $APP_HOME/gradle/wrapper/gradle-wrapper.jar https://raw.githubusercontent.com/gradle/gradle/v5.4.1/gradle/wrapper/gradle-wrapper.jar
-fi
-
Review comment:
Is this no longer needed at all? Did the usage of the gradle-wrapper.jar for gradle 5.6.4? I’m surprised that this line is no longer needed given that this jar is still referenced on the old line 71 / new line 67 (https://github.com/apache/iceberg/pull/2816/files#diff-e9721dc750619a21053ddea8a5d04929a608877d8c5daec1b57d243d3424e745R67)
Have you tested that this works on fresh system without the gradle wrapper jar present?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Rolly992 commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
Rolly992 commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r672344446
##########
File path: gradlew
##########
@@ -64,10 +64,6 @@ case "`uname`" in
;;
esac
-if [ ! -e $APP_HOME/gradle/wrapper/gradle-wrapper.jar ]; then
- curl -o $APP_HOME/gradle/wrapper/gradle-wrapper.jar https://raw.githubusercontent.com/gradle/gradle/v5.4.1/gradle/wrapper/gradle-wrapper.jar
-fi
-
Review comment:
I think I need to append this PR with the Gradle wrapper jar in another commit. Will do it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Rolly992 commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
Rolly992 commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r672344446
##########
File path: gradlew
##########
@@ -64,10 +64,6 @@ case "`uname`" in
;;
esac
-if [ ! -e $APP_HOME/gradle/wrapper/gradle-wrapper.jar ]; then
- curl -o $APP_HOME/gradle/wrapper/gradle-wrapper.jar https://raw.githubusercontent.com/gradle/gradle/v5.4.1/gradle/wrapper/gradle-wrapper.jar
-fi
-
Review comment:
I think I need to append this PR with the Gradle wrapper jar in another commit. Will do it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#issuecomment-882588565
@RussellSpitzer is this something that you could merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r668850532
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
Seems like a really small default heap? Any specific reason for changing this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#issuecomment-882588565
@RussellSpitzer is this something that you could merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r670380080
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -21,4 +21,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-5.4.1-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.4-bin.zip
Review comment:
fwiw I took a stab at upgrading to the latest Gradle version in https://github.com/apache/iceberg/pull/2826. We currently depend on another gradle plugin to be Gradle 7 compatible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] runningcode commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
runningcode commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r668861499
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
This is the default, automatically generated by the Gradle wrapper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Rolly992 commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
Rolly992 commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r669011316
##########
File path: gradle/wrapper/gradle-wrapper.properties
##########
@@ -21,4 +21,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-5.4.1-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.4-bin.zip
Review comment:
I've tried version 7.1.1. I don't remember everything that failed exactly, but there were some removed API which needed migration and something else, maybe plugins what needed version update as well.
To avoid broadening scope of changes with tasks not related to build cache, I didn't change major version here.
Though this looks interesting to me and I might try migrating to version 7 later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#issuecomment-882588565
@RussellSpitzer is this something that you could merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] big-guy commented on a change in pull request #2816: Gradle build cache optimization
Posted by GitBox <gi...@apache.org>.
big-guy commented on a change in pull request #2816:
URL: https://github.com/apache/iceberg/pull/2816#discussion_r670396832
##########
File path: gradlew
##########
@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
-DEFAULT_JVM_OPTS=""
+DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'
Review comment:
> If you’re optimizing for build cache (and for gradle runtimes in general), I would think raising this might be beneficial. Particularly, for compiling scala code I usually see it recommended that the gradle JVM get more memory.
@runningcode is correct.
There's the client VM (environment variables DEFAULT_JVM_ARGS, GRADLE_OPTS and JAVA_OPTS all affect this), the Gradle daemon (controlled by org.gradle.jvmargs which can be set in several ways) and the _worker processes_ used by compilers (built in default, but adjustable in the build script).
So to affect Scala compilation, you'd have to tweak the last one (worker process).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org