You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/07 19:48:34 UTC
[GitHub] [geode] demery-pivotal opened a new pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
demery-pivotal opened a new pull request #6280:
URL: https://github.com/apache/geode/pull/6280
Updated Geode's build system to use Gradle 6.8.3:
- Updated the gradle wrapper to use Gradle 6.8.3.
- Updated geode-assembly/build.gradle to make the ivy "repository"
compatible with Gradle 6.8.3.
- Changed ./gradle.properties to specify 'minimumGradleVersion=6.8'.
- Updated the expected-pom.xml file to the format that Gradle 6.8.3
produces.
- Updated all Gradle plugins to the latest versions.
- In buildSrc/build.gradle, added groovy source sets for testing, to
allow running tests of plugin code.
- Updated common "test isolation" code to be compatible with Gradle
6.8.3. See below for details.
- Updated the RepeatTest task to be compatible with Gradle 6.8.3. See
below for details.
- Rewrote the Dockerized test plugin to be compatible with Gradle 6.8.3.
See below for details.
- Added a new 'dunitDockerJVM' project property to allow developers run
tests in Docker on macOS. To do this, use -PtestJVM to identify the
mac's JVM, and use -PdunitDockerJVM to specify the JVM for the test
worker process running in Docker.
- Updated the BUILDING.md file for Gradle 6.8.3. As part of this I added
two versions of the Apache copyright notice to the etc dir: A plain
text version and an xml version that IntelliJ IDEA can import into its
copyright settings.
===== Test isolation code =====
Some code for isolating tests is used by the RepeatTest task and the
Dockerized test plugin. This code was updated to be compatible with
Gradle v6.8.3, and to somewhat reduce its dependence on Gradle internal
implementation details:
- WorkingDirectoryIsolator adjusts a given ProcessBuilder to give it a
unique working directory.
- AdjustableProcessLauncher applies a given Consumer (such as the
isolator, above) to adjust each ProcessBuilder before launching the
process.
- LauncherProxyWorkerProcessBuilder and
LauncherProcessWorkerProcessFactory forcefully update each Gradle exec
handle to use a given ProcessLauncher to launch test worker processes.
- Executers and Workers offer convenience methods for constructing
TestExecuters and ProcessWorker builders and factories.
===== RepeatTest task update =====
Geode's RepeatTest task uses a custom TestExecuter that was copied from
Gradle 5.5 (or earlier) and modified. This custom class is now updated
to be compatible with Gradle 6.8.3, and renamed from
OverriddenTestExecutor to RepeatableTestExecuter.
===== Dockerized test plugin rewrite =====
Removed the modified copy of pedjak's Dockerized test plugin.
Configuration
-------------
- Rewrote gradle/docker.gradle and renamed it as
gradle/multi-process-test.gradle.
- Moved the code that translates project properties into configuration
settings from the plugin to the config class. Renamed the config class
as DockerTestWorkerConfig.
- Moved code that configures command lines and environments for Docker
test workers based on project properties. Thise code was defiend as a
closure in a build script. It is now a method in
DockerTestWorkerConfig.
- Changed command line options that CI scripts pass to Gradle to run
tests in Docker:
- Specify the --max-workers Gradle option to configure the maximum
number of workers.
- Specify the 'testMaxParallelForks' project property to configure
the maximum number of forks for each test task.
- Remove the 'dunitParallelForks' option.
Implementation
--------------
The Dockerized test plugin uses these classes to launches processes in
Docker containers:
- DockerProcess overrides java.lang.Process to represent a test worker
process running in Docker.
- DockerProcessLauncher launches each test worker process in a Docker
container, and creates a DockerProcess to represent and manage it.
- Other classes described in the "Test isolation code" section, above.
The plugin overrides several of Gradle's internal classes to allow test
worker processes running in Docker containers to communicate with
Gradle:
- DockerConnectionAcceptor implements Gradle's ConnectionAcceptor, which
(among other responsibilities), produces a "multi-choice address" that
a test worker process can use to connect to Gradle's messaging server.
Gradle's default implementation produces multi-choice addresses that
processes in Docker containers cannot use.
- DockerMessagingServer implements Gradle's MessagingServer using
DockerConnectionAcceotpr.
- WildcardBindingInetAddressFactory overrides Gradle's
InetAddressFactory to instruct messaging servers to listen on an
address that processes in Dockerized containers can connect to.
Timeouts
--------
Due to a recent surge of CI problems due to delays in Docker operations,
I rewrote the plugins's timeout mechanism to make it configurable and
much more robust:
- Added a 'dockerTimeout' project property, defaulting to 5 minutes.
Each Docker operation will throw an exception if its duration exceeds
this value.
- Log a warning if any Docker operation takes longer than 1 minute. This
can help us to see how often significant delays occur in CI, while
still allowing tests to proceed.
- These exceptions and warnings identify the project for which test
worker process is being launched. This can help us to see whether
there are project-specific factors that affect the delays.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal merged pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
demery-pivotal merged pull request #6280:
URL: https://github.com/apache/geode/pull/6280
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on pull request #6280:
URL: https://github.com/apache/geode/pull/6280#issuecomment-815304354
I looked at the CPU and memory use for this PRs distributed test job, and we are getting lower CPU and memory pressure, which tells me we could crank up the allowed parallel threads and get the job runtime down from its current 2.5 hours.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6280:
URL: https://github.com/apache/geode/pull/6280#issuecomment-815340061
This pull request **fixes 1 alert** when merging 5a15ab0e08e3c34264859de316073e55708f8240 into 9964531d146c49a7094b8f534d6fa6468b14835e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-d787ceea78787bc6c9b3e933fa87ae5d201fdd59)
**fixed alerts:**
* 1 for Dereferenced variable may be null
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #6280:
URL: https://github.com/apache/geode/pull/6280#issuecomment-815317405
> I am not going to be able to review this in the near term. My review should not block this merge.
No worries Jake, @rhoughton-pivot's review alone will satisfy CODEOWNERS
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #6280:
URL: https://github.com/apache/geode/pull/6280#discussion_r609112699
##########
File path: gradle.properties
##########
@@ -79,7 +77,7 @@ org.gradle.configureondemand = false
org.gradle.daemon = true
org.gradle.jvmargs = -Xmx3g
org.gradle.parallel = true
-#org.gradle.workers.max = 3
+org.gradle.workers.max = 8
Review comment:
That's a mistake. I thought were were previously setting this to 8 here. I don't know why I thought that. I'll fix it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6280:
URL: https://github.com/apache/geode/pull/6280#issuecomment-815244025
This pull request **fixes 1 alert** when merging b668767d37c68c8fd598f096ddb9c487076c9bf7 into 35f8d9db9a01b3434f06b5af2b1d64d16363e800 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-5aeeac0cef6c9ffe942745cd78307c239fd5bf4a)
**fixed alerts:**
* 1 for Dereferenced variable may be null
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #6280: GEODE-8899: Upgrade to Gradle v6.8.3
Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #6280:
URL: https://github.com/apache/geode/pull/6280#discussion_r609108347
##########
File path: gradle.properties
##########
@@ -79,7 +77,7 @@ org.gradle.configureondemand = false
org.gradle.daemon = true
org.gradle.jvmargs = -Xmx3g
org.gradle.parallel = true
-#org.gradle.workers.max = 3
+org.gradle.workers.max = 8
Review comment:
Why are we setting max workers, instead of letting Gradle check the CPU count on the host?
##########
File path: buildSrc/src/main/java/org/apache/geode/gradle/testing/process/LauncherProxyWorkerProcessBuilder.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ *
+ */
+
+package org.apache.geode.gradle.testing.process;
+
+import static org.apache.geode.gradle.testing.process.Reflection.getField;
+import static org.apache.geode.gradle.testing.process.Reflection.getFieldValue;
+import static org.apache.geode.gradle.testing.process.Reflection.setFieldValue;
+
+import java.io.File;
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Proxy;
+import java.net.URL;
+import java.util.List;
+import java.util.Set;
+
+import org.gradle.api.Action;
+import org.gradle.api.logging.LogLevel;
+import org.gradle.process.internal.JavaExecHandleBuilder;
+import org.gradle.process.internal.worker.WorkerProcess;
+import org.gradle.process.internal.worker.WorkerProcessBuilder;
+import org.gradle.process.internal.worker.WorkerProcessContext;
+
+/**
+ * Wraps a worker process builder to make it use the given process launcher.
+ */
+public class LauncherProxyWorkerProcessBuilder implements WorkerProcessBuilder {
+ private final WorkerProcessBuilder delegate;
+ private final ProcessLauncher processLauncher;
+ private static Object processLauncherProxy;
+
+ public LauncherProxyWorkerProcessBuilder(WorkerProcessBuilder delegate,
+ ProcessLauncher processLauncher) {
+ this.delegate = delegate;
+ this.processLauncher = processLauncher;
+ }
+
+ @Override
+ public WorkerProcessBuilder applicationClasspath(Iterable<File> files) {
+ return delegate.applicationClasspath(files);
+ }
+
+ @Override
+ public Set<File> getApplicationClasspath() {
+ return delegate.getApplicationClasspath();
+ }
+
+ @Override
+ public WorkerProcessBuilder applicationModulePath(Iterable<File> files) {
+ return delegate.applicationModulePath(files);
+ }
+
+ @Override
+ public Set<File> getApplicationModulePath() {
+ return delegate.getApplicationModulePath();
+ }
+
+ @Override
+ public WorkerProcessBuilder setBaseName(String baseName) {
+ return delegate.setBaseName(baseName);
+ }
+
+ @Override
+ public String getBaseName() {
+ return delegate.getBaseName();
+ }
+
+ @Override
+ public WorkerProcessBuilder setLogLevel(LogLevel logLevel) {
+ return delegate.setLogLevel(logLevel);
+ }
+
+ @Override
+ public WorkerProcessBuilder sharedPackages(Iterable<String> packages) {
+ return delegate.sharedPackages(packages);
+ }
+
+ @Override
+ public Set<String> getSharedPackages() {
+ return delegate.getSharedPackages();
+ }
+
+ @Override
+ public JavaExecHandleBuilder getJavaCommand() {
+ return delegate.getJavaCommand();
+ }
+
+ @Override
+ public LogLevel getLogLevel() {
+ return delegate.getLogLevel();
+ }
+
+ @Override
+ public WorkerProcessBuilder sharedPackages(String... packages) {
+ return delegate.sharedPackages(packages);
+ }
+
+ @Override
+ public Action<? super WorkerProcessContext> getWorker() {
+ return delegate.getWorker();
+ }
+
+ @Override
+ public void setImplementationClasspath(List<URL> implementationClasspath) {
+ delegate.setImplementationClasspath(implementationClasspath);
+ }
+
+ @Override
+ public void setImplementationModulePath(List<URL> implementationModulePath) {
+ delegate.setImplementationModulePath(implementationModulePath);
+ }
+
+ @Override
+ public void enableJvmMemoryInfoPublishing(boolean shouldPublish) {
+ delegate.enableJvmMemoryInfoPublishing(shouldPublish);
+ }
+
+ /**
+ * Replaces the standard worker process's process launcher with this builder's launcher.
+ */
+ @Override
+ public WorkerProcess build() {
+ WorkerProcess workerProcess = delegate.build();
+ Object workerProcessDelegate = getFieldValue(workerProcess, "delegate");
+ Object execHandle = getFieldValue(workerProcessDelegate, "execHandle");
+ Class<?> processLauncherType = getField(execHandle, "processLauncher").getType();
+ setFieldValue(execHandle, "processLauncher", assignableProcessLauncher(processLauncherType));
+ return workerProcess;
+ }
+
+ /**
+ * Because the exec handle created by Gradle uses a classloader different from ours, we can't
+ * simply construct a Gradle {@code ProcessLauncher) to assign. Instead we create proxy, using the
Review comment:
Syntax error after `{@code ProcessLauncher`. Only caught it because of GitHub's syntax highlighting.
##########
File path: buildSrc/build.gradle
##########
@@ -17,39 +17,39 @@
*/
plugins {
- id 'java'
+ id 'java'
Review comment:
Looks like only indentation changed here. I guess `spotless` can't run on the `buildSrc` dir. Can you manually fix it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org