Skip to content
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

Hide bwc build output on success #42102

Merged
merged 10 commits into from May 16, 2019

Conversation

Projects
None yet
3 participants
@rjernst
Copy link
Member

commented May 11, 2019

Previously we used LoggedExec for running the internal bwc builds.
However, this had bad performance implications as all the output was
buffered into memory, thus we changed back to normal Exec. This commit
adds a useFileBuffer setting to LoggedExec which can be used for
commands with large amounts of output, and switches the bwc builds to
use this flag.

Hide bwc build output on success
Previously we used LoggedExec for running the internal bwc builds.
However, this had bad performance implications as all the output was
buffered into memory, thus we changed back to normal Exec. This commit
adds a `useFileBuffer` setting to LoggedExec which can be used for
commands with large amounts of output, and switches the bwc builds to
use this flag.
@elasticmachine

This comment has been minimized.

Copy link

commented May 11, 2019

@rjernst rjernst requested a review from mark-vieira May 11, 2019

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/bwc

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/1

@mark-vieira
Copy link
Contributor

left a comment

Just a couple of minor comments. Otherwise 💯 on this change 👍

ByteArrayOutputStream output = new ByteArrayOutputStream();
ByteArrayOutputStream error = new ByteArrayOutputStream();
this.useFileBuffer = getProject().getObjects().property(Boolean.class);
this.useFileBuffer.set(false); // default to in memory

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 13, 2019

Contributor

I believe the "convention" is to use convention() for setting default values on properties.

Show resolved Hide resolved buildSrc/src/main/minimumRuntime/org/elasticsearch/gradle/LoggedExec.java Outdated
Show resolved Hide resolved buildSrc/src/main/minimumRuntime/org/elasticsearch/gradle/LoggedExec.java Outdated
Show resolved Hide resolved buildSrc/src/main/minimumRuntime/org/elasticsearch/gradle/LoggedExec.java Outdated
Show resolved Hide resolved buildSrc/src/main/minimumRuntime/org/elasticsearch/gradle/LoggedExec.java Outdated

rjernst added some commits May 14, 2019

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Thanks @mark-vieira. I really liked your idea for not using afterEvaluate. I pushed e4b8a9b, can you take another look? I believe I also addressed your other big points.

rjernst added some commits May 14, 2019

@mark-vieira
Copy link
Contributor

left a comment

Changes look good! Just need to fix the stream close() bit and this is good to go.

Show resolved Hide resolved ...c/main/minimumRuntime/org/elasticsearch/gradle/LazyFileOutputStream.java
if (useFileBuffer) {
File spoolFile = new File(getProject().getBuildDir() + "/buffered-output/" + this.getName());
out = new LazyFileOutputStream(spoolFile);
outputReader = () -> Files.readString(spoolFile.toPath());

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 14, 2019

Contributor

Another potential optimization (and not a blocker) would be to define instead an outputLogger instead of outputReader which also handled the logging bit. This way the file-based once could stream from the spool file to the console instead of having to read the entire log file into memory.

Show resolved Hide resolved ...c/main/minimumRuntime/org/elasticsearch/gradle/LazyFileOutputStream.java

rjernst added some commits May 15, 2019

@mark-vieira

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Updates LGTM 👍

@rjernst rjernst merged commit bc0b0f5 into elastic:master May 16, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@rjernst rjernst deleted the rjernst:distro_tests7 branch May 16, 2019

rjernst added a commit that referenced this pull request May 16, 2019

Hide bwc build output on success (#42102)
Previously we used LoggedExec for running the internal bwc builds.
However, this had bad performance implications as all the output was
buffered into memory, thus we changed back to normal Exec. This commit
adds a `spoolOutput` setting to LoggedExec which can be used for
commands with large amounts of output, and switches the bwc builds to
use this flag.

Megamiun added a commit to Megamiun/elasticsearch that referenced this pull request May 18, 2019

Hide bwc build output on success (elastic#42102)
Previously we used LoggedExec for running the internal bwc builds.
However, this had bad performance implications as all the output was
buffered into memory, thus we changed back to normal Exec. This commit
adds a `spoolOutput` setting to LoggedExec which can be used for
commands with large amounts of output, and switches the bwc builds to
use this flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.