GEODE-8637: Give each test worker a unique working dir #5649
Conversation
demery-pivotal
requested review from
jake-at-work,
onichols-pivotal,
rhoughton-pivot and
smgoller
| Path workerWorkingDir = taskWorkingDir.resolve(workerWorkingDirName); | ||
|
|
||
| createWorkingDir(workerWorkingDir); | ||
| copyGemFirePropertiesFile(taskWorkingDir, workerWorkingDir); |
There was a problem hiding this comment.
I wonder if we need/want this gemfire.properties file anymore? As far as I'm concerned tests shouldn't be running with a gemfire.properties file lying around anyway?
I see we have TestPropertiesWriter that creates a properties file. But all it has is log-level=config, which is the default anyway...
There was a problem hiding this comment.
Just today I noticed that the non-repeat forms of these tasks don't write as properties file. If it's not needed, it would probably be better to delete the code that writes it.
There was a problem hiding this comment.
The gemfire.properties files, in all its incarnations, has been a thorn to me, because the writing of it causes a repack of geode-core.jar, which triggers all kinds of downstream effects.
There was a problem hiding this comment.
Do you have a reason to believe it's still needed? If you're not sure, I can do a run without it and see what explodes.
There was a problem hiding this comment.
I don't think it is? The one thing maybe it might do is prevent us from looking for a gemfire.properties file in other places (the classpath?, the user's homedir?). Could be worth a run if we can get rid of some code.
BTW, @rhoughton-pivot - I think maybe you are talking about writing the default properties file? That's different code I believe. But yeah, still seems like in that case maybe we could just check in a file or something.
There was a problem hiding this comment.
Hmmm. If the point of that file is to prevent looking in other places… removing it might change behavior on developers' machines, if they have a ~/gemfire.properties file. And a run in CI would not notice the problem.
There was a problem hiding this comment.
I am probably thinking of the gemfireVersion.properties :(
| public Action<WorkerProcessBuilder> getWorkerConfigurationAction() { | ||
| return workerProcessBuilder -> { | ||
| delegate.getWorkerConfigurationAction().execute(workerProcessBuilder); | ||
| JavaExecHandleBuilder javaCommand = workerProcessBuilder.getJavaCommand(); |
There was a problem hiding this comment.
Huh, I wonder if you could do something like javaCommand.setExecutable("docker") ... to run the workers in a docker container ;)
There was a problem hiding this comment.
That's a really interesting idea!
There was a problem hiding this comment.
I suspect it's not as simple as that, given that the Gradle client process needs to interact with the test worker process.
There was a problem hiding this comment.
Yeah, I'm sure it's a bit more complicated. But maybe all of the gradle internal APIs that we would need to use are already in use in this PR?
Anyway, I wasn't being that serious with this comment, mostly just teasing you since I know you're trying to get rid of docker.
There was a problem hiding this comment.
I got the tease, but maybe you didn't know: We're not trying to get rid of docker per se. We're trying to get rid of dependence on an old, unmaintained dockerizer plugin that's holding us back. Now that I'm turning my attention back port conflicts, I find your not-serious idea unreasonably tempting! I'm not hopeful, but I will look into it.
| Path workerWorkingDir = taskWorkingDir.resolve(workerWorkingDirName); | ||
|
|
||
| createWorkingDir(workerWorkingDir); | ||
| copyGemFirePropertiesFile(taskWorkingDir, workerWorkingDir); |
There was a problem hiding this comment.
The gemfire.properties files, in all its incarnations, has been a thorn to me, because the writing of it causes a repack of geode-core.jar, which triggers all kinds of downstream effects.
| private void copyGemFirePropertiesFile(Path taskWorkingDir, Path workerWorkingDir) { | ||
| Path taskPropertiesFile = taskWorkingDir.resolve(GEMFIRE_PROPERTIES); | ||
| if (!Files.exists(taskPropertiesFile)) { | ||
| return; |
There was a problem hiding this comment.
If taskPropertiesFile does not exist, will it affect javaCommand.setWorkingDir(workerWorkingDir); (line 78)? Or will it have some unexpected behavior when running tests?
There was a problem hiding this comment.
If the file is missing, that doesn't indicate a problem. It's deliberate. Our Gradle build puts that file there for some test tasks (e.g. distributedTest) but not for others (e.g. repeatDistributedTest). I don't know why our Gradle build writes the file for some tasks and not others, but that's the current behavior.
The if statement accounts for Gradle writing the file for some tasks and not for others.
The effect is that if the file was in the test JVM's common working directory before this PR, it will be in the JVM's unique working directory after. And if the file was absent from the test JVM's working directory before the PR, it will be absent after.
demery-pivotal
force-pushed
the
geode-8637/unique-working-dir
branch
from
5fa02e9
to
167400f
Compare
onichols-pivotal
added
the
windows
demery-pivotal
force-pushed
the
geode-8637/unique-working-dir
branch
3 times, most recently
from
0f6994d
to
485f5dc
Compare
Before running a test task, wrap its TestFramework in a wrapper that creates a unique working dir for each new test worker.
demery-pivotal
force-pushed
the
geode-8637/unique-working-dir
branch
from
485f5dc
to
c7ccb84
Compare
Before running a test task, wrap its TestFramework in a wrapper that creates a unique working dir for each new test worker.
The affected test tasks are: