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

Store build.log in log directory #818

Merged
merged 4 commits into from Nov 19, 2019

Conversation

danielmitterdorfer
Copy link
Member

With this commit we move Rally's build log from a race-specific
directory to its log directory. This file captures Gradle's output when
users are benchmarking a source build of Elasticsearch. By moving this
file to the log directory it is better visible to users (already in the
same location as other log files).

With this commit we move Rally's build log from a race-specific
directory to its log directory. This file captures Gradle's output when
users are benchmarking a source build of Elasticsearch. By moving this
file to the log directory it is better visible to users (already in the
same location as other log files).
@danielmitterdorfer danielmitterdorfer added enhancement Improves the status quo :Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch breaking Non-backwards compatible change labels Nov 18, 2019
@danielmitterdorfer danielmitterdorfer added this to the 1.4.0 milestone Nov 18, 2019
@danielmitterdorfer
Copy link
Member Author

Marked as breaking as the build log file moves and thus it may have an impact on users.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this works great. I left a change request for the logrotate conf because in-line comments actually prohibit it from working. Otherwise LGTM, I don't think there's a new for another review cycle.

@@ -135,6 +135,18 @@ The log file will not be rotated automatically as this is problematic due to Ral
notifempty # don't attempt to rotate empty ones
}

If you are benchmarking source builds of Elasticsearch, Rally will write the log output of the build to ``~/.rally/logs/build.log``. Similarly to the configuration for ``rally.log``, you can use the following ``logrotate`` configuration as a starting point::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reading this first time, it may not be entirely clear what the logrotate config is for, I suggest we rephrase:

you can use the following logrotate configuration as a starting point::

to

you can use the following logrotate configuration to restrict the volume of the log file::

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that can be inferred based on context (i.e. reading the paragraph above about rally.log).

If you are benchmarking source builds of Elasticsearch, Rally will write the log output of the build to ``~/.rally/logs/build.log``. Similarly to the configuration for ``rally.log``, you can use the following ``logrotate`` configuration as a starting point::

/home/user/.rally/logs/build.log {
daily # rotate daily
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In-line comments don't work. They should be in a separate line.

(From man logrotate

Note that comments may appear anywhere in the config file as long as the first non-whitespace character on the line is a #.
)

We need to update also the example for rally.log.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in dd68f38.

@dliappis dliappis self-requested a review November 19, 2019 10:25
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danielmitterdorfer
Copy link
Member Author

Thanks!

@danielmitterdorfer danielmitterdorfer merged commit 65e31c4 into elastic:master Nov 19, 2019
@danielmitterdorfer danielmitterdorfer deleted the move-build-log branch November 19, 2019 10:25
danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this pull request Nov 29, 2019
With this commit we continously override a potentially existing
Elasticsearch build log. Keeping a history of build logs is not
necessary as it is only relevant to investigate on failures. Thus a
history only adds complexity (e.g. log rotation setup) for little value.

Relates elastic#818
danielmitterdorfer added a commit that referenced this pull request Dec 2, 2019
With this commit we continously override a potentially existing
Elasticsearch build log. Keeping a history of build logs is not
necessary as it is only relevant to investigate on failures. Thus a
history only adds complexity (e.g. log rotation setup) for little value.

Relates #818
Relates #832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch breaking Non-backwards compatible change enhancement Improves the status quo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants