Skip to content

[DOCS] Document throttle_period_in_millis for watcher actions #45607

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 19, 2019
Merged

[DOCS] Document throttle_period_in_millis for watcher actions #45607

merged 3 commits into from
Aug 19, 2019

Conversation

@jrodewig
Copy link
Contributor

@jrodewig jrodewig commented Aug 15, 2019 •
edited
Loading

@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 15, 2019

Pinging @elastic/es-core-features

@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 15, 2019

Pinging @elastic/es-docs

@jrodewig jrodewig mentioned this pull request Aug 15, 2019
Closed
@jrodewig
Copy link
Contributor Author

jrodewig commented Aug 15, 2019

@elasticmachine run elasticsearch-ci/2

@hub-cap hub-cap self-requested a review August 15, 2019 15:28
szabosteve
szabosteve approved these changes Aug 16, 2019
View reviewed changes
Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

LGTM.

hub-cap
hub-cap suggested changes Aug 16, 2019
View reviewed changes
Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

other than changing the wording on the overrides, this is LGTM.

x-pack/docs/en/rest-api/watcher/put-watch.asciidoc Outdated
for this is 5 seconds. This default can be changed in the
config file with the setting `xpack.watcher.throttle.period.default_period`.
config file with the setting
`xpack.watcher.throttle.period.default_period`. If
Copy link
Contributor

@hub-cap hub-cap Aug 16, 2019

Choose a reason for hiding this comment

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

Hey i did a quick eyeball of the code here and it looks like this is not correct. The code is basically "last one wins" due to the for loop + non final variable assignment when parsing the watch.

So if you have

"throttle_period_in_millis": "7000",
"throttle_period": "5s",

the GET watch API returns throttle_period_in_millis":5000,

and if you have the opposite,

"throttle_period": "5s",
"throttle_period_in_millis": "7000",

the GET watch API returns throttle_period_in_millis":7000,

Copy link
Contributor

@hub-cap hub-cap Aug 16, 2019

Choose a reason for hiding this comment

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

Here is the code im talking about for reference,

https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java#L159-L163

Copy link
Contributor Author

@jrodewig jrodewig Aug 16, 2019

Choose a reason for hiding this comment

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

Tricky! I did look at that code, but I incorrectly assumed the first matching else if would win.

I reversed the override statements with cfe41d9.

Thanks for catching this @hub-cap.

Copy link
Contributor

@hub-cap hub-cap Aug 16, 2019

Choose a reason for hiding this comment

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

So its not about one overriding the other, its about which one is declared first in the watch. if you declare both, the last one overrides the first one, so you can see in the example above that when i declared them both, which i did in 2 watches, the output was different based on which order they were declared in.

Copy link
Contributor Author

@jrodewig jrodewig Aug 16, 2019

Choose a reason for hiding this comment

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

🤦‍♂ Sorry about that. Guess I'm feeling dense today.

Corrected again with deaef45.

x-pack/docs/en/rest-api/watcher/put-watch.asciidoc Outdated

| `throttle_period_in_millis` | Minimum time in milliseconds between actions
being run. Defaults to `5000`. If provided, this
value overrides the `throttle_period` value.
Copy link
Contributor

@hub-cap hub-cap Aug 16, 2019

Choose a reason for hiding this comment

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

same here WRT the overrides.

Copy link
Contributor Author

@jrodewig jrodewig Aug 16, 2019

Choose a reason for hiding this comment

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

Fixed with cfe41d9.

Copy link
Contributor Author

@jrodewig jrodewig Aug 16, 2019

Choose a reason for hiding this comment

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

Corrected again with deaef45.

Copy link
Contributor

@hub-cap hub-cap Aug 16, 2019

Choose a reason for hiding this comment

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

💥

@jrodewig jrodewig merged commit 5d65efe into elastic:master Aug 19, 2019
@jrodewig jrodewig deleted the document-throttle-period-millis branch August 19, 2019 12:27
jrodewig added a commit that referenced this pull request Aug 19, 2019
@jrodewig
[DOCS] Document throttle_period_in_millis for watcher actions (#45607)
4b93251
jrodewig added a commit that referenced this pull request Aug 19, 2019
@jrodewig
[DOCS] Document throttle_period_in_millis for watcher actions (#45607)
c1178ee
jrodewig added a commit that referenced this pull request Aug 19, 2019
@jrodewig
[DOCS] Document throttle_period_in_millis for watcher actions (#45607)
bac3043
jrodewig added a commit that referenced this pull request Aug 19, 2019
@jrodewig
[DOCS] Document throttle_period_in_millis for watcher actions (#45607)
0268295
jrodewig added a commit that referenced this pull request Aug 19, 2019
@jrodewig
[DOCS] Document throttle_period_in_millis for watcher actions (#45607)
b11b53f
jrodewig added a commit that referenced this pull request Aug 19, 2019
@jrodewig
[DOCS] Document throttle_period_in_millis for watcher actions (#45607)
534ade8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szabosteve szabosteve szabosteve approved these changes

@rjernst rjernst Awaiting requested review from rjernst

@lcawl lcawl Awaiting requested review from lcawl

@laraMorenoIgle laraMorenoIgle Awaiting requested review from laraMorenoIgle

+1 more reviewer

@hub-cap hub-cap hub-cap approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Data Management/Watcher >docs General docs changes v6.8.3 v7.0.2 v7.1.2 v7.2.2 v7.3.1 v7.4.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[DOCS] Add examples in watchers definitions that uses throttle_period_in_millis

5 participants

@jrodewig @elasticmachine @hub-cap @szabosteve @jakelandis