[Transform] add throttling #56007

hendrikmuhs · 2020-04-30T09:25:54Z

add throttling to transform, throttling will slow down search requests by delaying the execution based on a documents per second metric. All details can be found in #54862

todo:

update transform action compat
- transport serialization
- rolling upgrade test
deprecate max_page_search_size in pivotconfig
settings should be merge-able
~~stats returning docs/s~~ (-> separate PR)

elasticmachine · 2020-04-30T09:25:57Z

Pinging @elastic/ml-core (:ml/Transform)

benwtrent · 2020-05-04T12:48:58Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/SettingsConfig.java

+
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
+
+public class SettingsConfig implements Writeable, ToXContentObject {


I am not a big fan of a settings clause in the configuration.

The whole configuration is settings. It's settings for a transform.

Additionally, if we are ever going to say there are more transforms than pivot, this means those other transforms will have to abide by whatever settings are in this object.

It seems to me that these options should be in the pivot configuration.

Perhaps ThrottlingConfig

As max_page_search_size is already in PivotConfig it is tempting to add docs_per_second as a loose field in PivotConfig and avoid deprecating PivotConfig.max_page_search_size and the associated BWC code. Possibly I am just a lazy programmer

...m/src/main/java/org/elasticsearch/xpack/transform/action/TransportUpdateTransformAction.java

benwtrent · 2020-05-04T13:08:17Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/SettingsConfig.java

+         * @return the {@link Builder} with requestsPerSecond set.
+         */
+        public Builder setRequestsPerSecond(Float docsPerSecond) {
+            this.docsPerSecond = docsPerSecond;


It seems to me that a null passed in here is where you want to set the default values back. Otherwise, it becomes complicated to know when a null means "not supplied" vs "revert to default"

Similar comment for setMaxPageSearchSize

I just found this to be a problem in hlrc (and fixed it there, not pushed yet).

However I wonder if this is really a problem in this place, both setters are not used, but only update. Maybe I rename this class to Updater and remove the setters.

...n/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/TransformIndexer.java

davidkyle

All looks good to me. I think this is practically complete isn't it?

.../core/src/main/java/org/elasticsearch/xpack/core/transform/action/UpdateTransformAction.java

davidkyle · 2020-05-04T14:20:30Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/SettingsConfig.java

+
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
+
+public class SettingsConfig implements Writeable, ToXContentObject {


Perhaps ThrottlingConfig

As max_page_search_size is already in PivotConfig it is tempting to add docs_per_second as a loose field in PivotConfig and avoid deprecating PivotConfig.max_page_search_size and the associated BWC code. Possibly I am just a lazy programmer

...e/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigUpdate.java

...t-high-level/src/main/java/org/elasticsearch/client/transform/transforms/SettingsConfig.java

.../core/src/main/java/org/elasticsearch/xpack/core/transform/action/UpdateTransformAction.java

hendrikmuhs · 2020-05-04T20:29:13Z

run elasticsearch-ci/bwc

hendrikmuhs · 2020-05-05T05:24:56Z

Resetting to defaults now works as this:

parsers treat explicit null values with magic (default) values
for hlrc explicit null is written as null in toXContent, an empty value is not written (-> no leak of magic values)
for the server code the explicit null is not written with toXContent (to not endup with null values in the config index)
update handles explicit null as reset to default
the magic defaults have values (<0) that are handled in the implementation as if their would not be set, so treated as defaults
for runtime update the old and new setting is compared and action is only taken if they change

There are some corner cases and things to improve, I like to address them as separate PR:

if you have an old < 7.8 transform with a Pivot.max_page_search_size a reset to null will not reset to the default, but to Pivot.max_page_search_size, I plan to handle this in the update code and get rid of Pivot.max_page_search_size` if a config is rewritten due to update (maybe even for an empty update call)
its possible to enter negative values for max_page_search_size and docs_per_second, this is treated as default, however it would be better to disallow anything <-1.

davidkyle

LGTM

The logic of resetting default values with an explicit null is quite complicated could you add a yml test to cover that please.

hendrikmuhs · 2020-05-05T09:23:48Z

The logic of resetting default values with an explicit null is quite complicated could you add a yml test to cover that please.

This is tested in all possible permutations here:

https://github.com/elastic/elasticsearch/pull/56007/files#diff-39339aadd414df66d2f5bdf71cf0f282R297

(hlrc, not yml, given that hlrc tests ensure I send null i think that should be covered)

add throttling to transform, throttling will slow down search requests by delaying the execution based on a documents per second metric. fixes elastic#54862

add throttling to transform, throttling will slow down search requests by delaying the execution based on a documents per second metric. fixes #54862

add documentation for throttling, added in #56007

fixes the page size reported after moving page size to settings(#56007) and adds documents per second(throttling) to the output. fixes #56498

…7871) fixes the page size reported after moving page size to settings(elastic#56007) and adds documents per second(throttling) to the output. fixes elastic#56498

fixes the page size reported after moving page size to settings(#56007) and adds documents per second(throttling) to the output. fixes #56498

hendrikmuhs added >enhancement WIP v8.0.0 :ml/Transform Transform v7.8.0 labels Apr 30, 2020

hendrikmuhs mentioned this pull request Apr 30, 2020

[Transform] Transform throttling part2.1 #55672

Closed

3 tasks

hendrikmuhs force-pushed the transform-throttling-part2.3 branch from e8964c9 to d9af494 Compare April 30, 2020 15:06

hendrikmuhs marked this pull request as ready for review April 30, 2020 15:24

hendrikmuhs removed the WIP label May 4, 2020

benwtrent self-requested a review May 4, 2020 12:24

benwtrent reviewed May 4, 2020

View reviewed changes

davidkyle reviewed May 4, 2020

View reviewed changes

Hendrik Muhs added 10 commits May 5, 2020 07:02

add throttling API's for transform

ba9b3ed

make settings partially updateable

40f9d41

assert version 8.0.0 (7.8.0 after backport)

a293ae1

enhance rolling upgrade test with update calls

1e0f041

add settings to transformconfigupdate in hlrc

a5e0e78

handle settings defaults in hlrc and server

a210961

apply review suggestion

e286e7a

handle page size update if resetting to null

96e75ff

add an integration test for updating at runtime

d7e9634

apply review suggestions

65586e3

hendrikmuhs force-pushed the transform-throttling-part2.3 branch from ec83eeb to 65586e3 Compare May 5, 2020 05:03

add missing quote

828c421

hendrikmuhs mentioned this pull request May 5, 2020

[DOC] document transform settings and docs_per_second #56178

Merged

davidkyle approved these changes May 5, 2020

View reviewed changes

hendrikmuhs merged commit 33f134f into elastic:master May 5, 2020

hendrikmuhs deleted the transform-throttling-part2.3 branch May 5, 2020 09:34

hendrikmuhs mentioned this pull request May 5, 2020

[7.x][Transform] add throttling (#56007) #56184

Merged

hendrikmuhs pushed a commit that referenced this pull request May 5, 2020

[7.x][Transform] add throttling (#56007) (#56184)

e177a38

add throttling to transform, throttling will slow down search requests by delaying the execution based on a documents per second metric. fixes #54862

hendrikmuhs added the release highlight label May 8, 2020

This was referenced May 11, 2020

[Transform] cat transforms returns deprecated max_page_search_size #56498

Closed

[Transform] update reset resets to old max_page_search_size #56499

Closed

hendrikmuhs pushed a commit that referenced this pull request May 11, 2020

[DOC] document transform settings and docs_per_second (#56178)

a73511e

add documentation for throttling, added in #56007

hendrikmuhs pushed a commit that referenced this pull request May 11, 2020

[DOC] document transform settings and docs_per_second (#56178)

c098561

add documentation for throttling, added in #56007

hendrikmuhs pushed a commit that referenced this pull request May 11, 2020

[DOC] document transform settings and docs_per_second (#56178)

d852386

add documentation for throttling, added in #56007

russcam mentioned this pull request May 29, 2020

7.8.0 Meta ticket elastic/elasticsearch-net#4718

Closed

17 tasks

hendrikmuhs mentioned this pull request Jun 9, 2020

[Transform] fix page size return in cat transform, add dps #57871

Merged

hendrikmuhs pushed a commit that referenced this pull request Jun 10, 2020

[Transform] fix page size return in cat transform, add dps (#57871)

75ea6a4

fixes the page size reported after moving page size to settings(#56007) and adds documents per second(throttling) to the output. fixes #56498

hendrikmuhs pushed a commit that referenced this pull request Jun 10, 2020

[Transform] fix page size return in cat transform, add dps (#57871)

95bd7b6

fixes the page size reported after moving page size to settings(#56007) and adds documents per second(throttling) to the output. fixes #56498

hendrikmuhs mentioned this pull request Jun 10, 2020

[7.8][Transform] fix page size return in cat transform, add dps #57912

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

lcawl mentioned this pull request Nov 26, 2021

Adds missing details in the create transform API elastic/elasticsearch-specification#1067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transform] add throttling #56007

[Transform] add throttling #56007

hendrikmuhs commented Apr 30, 2020 •

edited

Loading

elasticmachine commented Apr 30, 2020

benwtrent May 4, 2020

davidkyle May 4, 2020

benwtrent May 4, 2020

hendrikmuhs May 4, 2020

davidkyle left a comment

davidkyle May 4, 2020

hendrikmuhs commented May 4, 2020

hendrikmuhs commented May 5, 2020

davidkyle left a comment

hendrikmuhs commented May 5, 2020


		import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

		public class SettingsConfig implements Writeable, ToXContentObject {

[Transform] add throttling #56007

[Transform] add throttling #56007

Conversation

hendrikmuhs commented Apr 30, 2020 • edited Loading

elasticmachine commented Apr 30, 2020

benwtrent May 4, 2020

Choose a reason for hiding this comment

davidkyle May 4, 2020

Choose a reason for hiding this comment

benwtrent May 4, 2020

Choose a reason for hiding this comment

hendrikmuhs May 4, 2020

Choose a reason for hiding this comment

davidkyle left a comment

Choose a reason for hiding this comment

davidkyle May 4, 2020

Choose a reason for hiding this comment

hendrikmuhs commented May 4, 2020

hendrikmuhs commented May 5, 2020

davidkyle left a comment

Choose a reason for hiding this comment

hendrikmuhs commented May 5, 2020

hendrikmuhs commented Apr 30, 2020 •

edited

Loading