-
Notifications
You must be signed in to change notification settings - Fork 313
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
Omit needless words in track tutorial #499
Omit needless words in track tutorial #499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this massive simplification effort!
I left a few comments.
docs/adding_tracks.rst
Outdated
* The numbers below the ``documents`` property are needed to verify integrity and provide progress reports. Determine the correct document count with ``wc -l documents.json`` and the size in bytes with ``stat -f "%z" documents.json``. | ||
* You can add as many queries as you want. We use the `official Python Elasticsearch client <http://elasticsearch-py.readthedocs.org/>`_ to issue queries. | ||
|
||
The numbers below the ``documents`` property are needed to verify integrity and provide progress reports. Determine the correct document count with ``wc -l documents.json`` and the size in bytes with ``stat -f "%z" documents.json``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "below" could be replaced by "under" as my first instinct is to look at the next line/property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
docs/adding_tracks.rst
Outdated
|
||
* Iteration-based tasks will run at most one warmup iteration and one measurement iteration. | ||
* Time-period-based tasks will run for at most 10 seconds without any warmup. | ||
You can check your track very quickly for syntax errors when you invoke Rally with ``--test-mode``. Rally postprocesses its internal track representation then: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then
at the end of the sentence sounds weird to me. How about:
Rally now postprocesses its internal track representation:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
Rally postprocesses its internal track representation as follows:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
docs/adding_tracks.rst
Outdated
|
||
Rally will postprocess all data file names of a track. So instead of ``documents.json``, Rally will attempt to find ``documents-1k.json`` and will assume it contains 1.000 documents. However, you need to prepare these data files otherwise this test mode is not supported. | ||
* Iteration-based tasks run at most one warmup iteration and one measurement iteration. | ||
* Time-period-based tasks run for at most 10 seconds without warmup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run for at most
--> run, at most, for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I interpret the "at most" as a restrictive clause in this case. Hence we should not enclose it in commas. So I'd write: "run at most for".
docs/adding_tracks.rst
Outdated
@@ -409,7 +403,7 @@ If you want to reuse operation definitions across challenges, you can also defin | |||
|
|||
Note how we reference to the operations by their name (e.g. ``create``, ``bulk-index``, ``force-merge`` or ``query-match-all``). | |||
|
|||
If your track consists of multiple challenges, it can be cumbersome to include them all explicitly. Therefore Rally brings a ``collect`` helper that collects all related files for you. Let's adapt our track to use it:: | |||
It can be cumbersome to include multiple challenges explicitly so you can use Rally's ``collect`` helper instead:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds a little strange (to me). What about:
You can also use Rally's
collect
helper to simplify including multiple challenges explicitly:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
docs/adding_tracks.rst
Outdated
|
||
.. note:: | ||
|
||
Rally's log file contains the fully rendered track after it has loaded it successfully. | ||
|
||
.. note:: | ||
|
||
If you define multiple challenges, Rally will run the challenge where ``default`` is set to ``true``. If you want to run a different challenge, provide the command line option ``--challenge=YOUR_CHALLENGE_NAME``. | ||
If you define multiple challenges, Rally runs the challenge where ``default`` is set to ``true``. If you want to run a different challenge, provide the command line option ``--challenge=YOUR_CHALLENGE_NAME``. | ||
|
||
You can even use `Jinja2 variables <http://jinja.pocoo.org/docs/2.9/templates/#assignments>`_ but you need to import the Rally helpers a bit differently then. You also need to declare all variables before the ``import`` statement:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not affected by this PR, but now that I saw it, should we re-order then like:
but then you need to import the Rally helpers a bit differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've changed it.
docs/adding_tracks.rst
Outdated
|
||
Advanced topics | ||
--------------- | ||
|
||
Template Language | ||
^^^^^^^^^^^^^^^^^ | ||
|
||
Rally uses `Jinja2 <http://jinja.pocoo.org/docs/dev/>`_ as template language. This allows you to use Jinja2 expressions in track files. | ||
Rally uses `Jinja2 <http://jinja.pocoo.org/docs/dev/>`_ as template language so you can use Jinja2 expressions in track files. | ||
|
||
|
||
Extension Points | ||
"""""""""""""""" | ||
|
||
Rally also provides a few extension points to Jinja2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just a few extensions
(without points)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's simpler indeed. I also changed the heading accordingly.
@@ -568,7 +565,7 @@ You can find an example in the http_logs track:: | |||
} | |||
} | |||
|
|||
The data set that is used in the http_logs track starts on 26-04-1998 but we want to ignore the first few days for this query, so we start on 15-05-1998. The expression ``{{'15-05-1998' | days_ago(now)}}`` yields the difference in days between now and the fixed start date and allows us to benchmark time range queries relative to now with a predetermined data set. | |||
The data set that is used in the ``http_logs`` track starts on 26-04-1998 but we want to ignore the first few days for this query, so we start on 15-05-1998. The expression ``{{'15-05-1998' | days_ago(now)}}`` yields the difference in days between now and the fixed start date and allows us to benchmark time range queries relative to now with a predetermined data set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Substitute yields
with just provides (or returns)? Not every track author may be familiar with Python's yield
and generators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the word "yield" here in a mathematical, not in a software engineering context.
@@ -605,7 +602,7 @@ First, define the name of your parameter source in the operation definition:: | |||
"professions": ["mechanic", "physician", "nurse"] | |||
} | |||
|
|||
Rally will recognize the parameter source and looks then for a file ``track.py`` in the same directory as the corresponding JSON file. This file contains the implementation of the parameter source:: | |||
Rally recognizes the parameter source and looks for a file ``track.py`` next to ``track.json``. This file contains the implementation of the parameter source:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace next
with:
in the same directory (or folder) as
track.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "next to" is just as clear?
docs/adding_tracks.rst
Outdated
@@ -786,11 +775,11 @@ Then create a file ``track.py`` next to ``track.json`` and implement the followi | |||
The function ``percolate`` is the actual runner and takes the following parameters: | |||
|
|||
* ``es``, which is the Elasticsearch Python client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think which
can be skipped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've changed it.
docs/adding_tracks.rst
Outdated
@@ -786,11 +775,11 @@ Then create a file ``track.py`` next to ``track.json`` and implement the followi | |||
The function ``percolate`` is the actual runner and takes the following parameters: | |||
|
|||
* ``es``, which is the Elasticsearch Python client | |||
* ``params`` which is a dict of parameters provided by its corresponding parameter source. Treat this parameter as read only and do not attempt to write to it. | |||
* ``params`` which is a ``dict`` of parameters provided by its corresponding parameter source. Treat this parameter as read-only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again which
I think is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've changed it.
Thanks for iterating, LGTM. |
No description provided.