changing hard coded 10k page size limit to 65k #74651

Esduard · 2021-06-28T19:11:09Z

Closes #57719. I managed to change the lines to a hardcoded 65K. I'm still trying to understand how settings work to remove the hardcoded lines though. I would appreciate If someone could give me some directions .

benwtrent · 2021-06-28T19:26:11Z

jenkins, test this please

elasticmachine · 2021-06-28T19:26:27Z

Pinging @elastic/ml-core (Team:ML)

hendrikmuhs

@Esduard Thanks for your contribution, looks great!

Some tests are failing and require changes, e.g. these 2 occurrences of 10,000 in this test suite

Can you also adjust the docs in https://github.com/elastic/elasticsearch/blob/master/docs/reference/rest-api/common-parms.asciidoc and replace 10,000 with 65535?

hendrikmuhs · 2021-06-29T08:55:41Z

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

    public ActionRequestValidationException validate(ActionRequestValidationException validationException) {
        // TODO: make this dependent on search.max_buckets
-        if (maxPageSearchSize != null && (maxPageSearchSize < 10 || maxPageSearchSize > 10_000)) {
+        if (maxPageSearchSize != null && (maxPageSearchSize < 10 || maxPageSearchSize > 65_536)) {


The limit is 65_535. You might stumbled upon https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket.html, which says 65_536. I think this is wrong, I will check this.

I turns out 65_536 is correct but the referenced page is wrong.

To avoid future confusion, it would be nice to replace 65_536 with this constant: MultiBucketConsumerService.DEFAULT_MAX_BUCKETS

But this is nice to have, I can do this change myself as well.

@hendrikmuhs, Thank you for the feedback! I took the 65_536 value from the issue #70706.

Don't worry. I am making these changes and will commit soon

hendrikmuhs · 2021-06-29T08:56:01Z

.../core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/pivot/PivotConfig.java


    public ActionRequestValidationException validate(ActionRequestValidationException validationException) {
-        if (maxPageSearchSize != null && (maxPageSearchSize < 10 || maxPageSearchSize > 10_000)) {
+        if (maxPageSearchSize != null && (maxPageSearchSize < 10 || maxPageSearchSize > 65_536)) {


~~same here, 65535~~ redacted, 65536 is correct

hendrikmuhs · 2021-06-29T08:58:48Z

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

+        if (maxPageSearchSize != null && (maxPageSearchSize < 10 || maxPageSearchSize > 65_536)) {
            validationException = addValidationError(
-                "settings.max_page_search_size [" + maxPageSearchSize + "] must be greater than 10 and less than 10,000",
+                "settings.max_page_search_size [" + maxPageSearchSize + "] must be greater than 10 and less than 65,536",


you found a bug, this error message is mathematically not correct, because 10 and 10,000 are allowed values. I am checking this and might come back with an improved wording.

Can you use this error message:

"settings.max_page_search_size [" + maxPageSearchSize + "] is out of range. The minimum value is 10 and the maximum is " + MultiBucketConsumerService.DEFAULT_MAX_BUCKETS

import 'max_buckets' constant

updating documentation with new max page size

Esduard · 2021-06-30T03:41:03Z

@hendrikmuhs Thank you once again for reviewing my commits. I managed to change the tests to fit the new page limit and changed the documentation as well. My only dificulty is dealing with bwc tests, the project just won't resolve some dependencies.

hendrikmuhs · 2021-06-30T06:37:30Z

jenkins, test this please

hendrikmuhs · 2021-06-30T06:49:53Z

jenkins, test this please

hendrikmuhs · 2021-06-30T06:51:14Z

jenkins, test this please

hendrikmuhs · 2021-06-30T08:22:22Z

@elasticmachine update branch

hendrikmuhs · 2021-06-30T08:22:42Z

jenkins, test this please

Esduard · 2021-06-30T10:20:55Z

I ran "gradlew :x-pack:plugin:yamlRestTest" on my machine trying to recreate this test faliure of jenkins but wasn't able to. The test was a success.

hendrikmuhs · 2021-06-30T11:47:15Z

@Esduard Thanks for iterating on this. I exploited our timezone difference and fixed some stuff.

The remaining failure is probably benign. The test executes code from the 7.x branch, that's why it is not a bug that this fails. I will check with the team how we proceed here and will get back to you.

hendrikmuhs · 2021-06-30T14:14:20Z

@Esduard The elasticsearch-ci/rest-compatibility test checks out the test code from the 7.x branch and tests it against this branch. That means we can't fix it in this branch, but the test can be disabled.

Can you change this file https://github.com/Esduard/elasticsearch/blob/master/x-pack/plugin/build.gradle#L170

and add transform/transforms_crud/Test put config with invalid pivot size to restTestBlacklist, best with a comment on top, e.g. // disabled until backport of #74651 has finished

Once jenkins is happy with it, I can take care of the rest: the backport and re-enabling the test on master.

Esduard · 2021-06-30T14:33:34Z

@hendrikmuhs I understand, thanks for the explanation. I have commited a new gradle.build with the pivot size test blacklisted for jenkins sake.

hendrikmuhs · 2021-06-30T14:34:42Z

jenkins, test this please

hendrikmuhs · 2021-06-30T14:59:47Z

@elasticmachine update branch

hendrikmuhs · 2021-06-30T15:00:01Z

jenkins, test this please

hendrikmuhs · 2021-07-01T14:24:39Z

Great, this looks good to me now!

Thanks for your contribution @Esduard! I will merge it in the next days.

…) (#74925) changes hard coded 10k page size limit to 65k(default for `search.max_bucket`) Closes #57719

re-enable test after backport of #74651

changing hard coded 10k page size limit to 65k

cf89967

elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Jun 28, 2021

Esduard mentioned this pull request Jun 28, 2021

[Transform] increase page size limit to 65k #57719

Closed

benwtrent added the :ml/Transform Transform label Jun 28, 2021

elasticmachine added the Team:ML Meta label for the ML team label Jun 28, 2021

hendrikmuhs reviewed Jun 29, 2021

View reviewed changes

Esduard added 3 commits June 30, 2021 00:13

Importing Variable Max Buckets

249c954

import 'max_buckets' constant

updating test cases for page size limit

9bdef77

updating documentation

3bbe358

updating documentation with new max page size

Hendrik Muhs added 3 commits June 30, 2021 08:46

fix checkstyle line length

be526ff

fix checkstyle line length

ed6473c

fix checkstyle

fadb504

Update PivotConfig.java

ebb831b

Merge branch 'master' into master

3fce9e3

blacklisting invalid pivot size test temporarily

4255f0d

Merge branch 'master' into master

73bd1bf

hendrikmuhs merged commit b3e171b into elastic:master Jul 5, 2021

hendrikmuhs mentioned this pull request Jul 5, 2021

[7.x][Transform] changing hard coded 10k page size limit to 65k (#74651) #74925

Merged

hendrikmuhs added v7.15.0 v8.0.0 labels Jul 5, 2021

hendrikmuhs pushed a commit to hendrikmuhs/elasticsearch that referenced this pull request Jul 5, 2021

re-enable page size rest test after backport elastic#74651

d7f5aae

hendrikmuhs mentioned this pull request Jul 5, 2021

[Transform] re-enable page size rest test after backport #74651 #74927

Merged

hendrikmuhs pushed a commit that referenced this pull request Jul 5, 2021

[7.x][Transform] changing hard coded 10k page size limit to 65k (#74651…

3092402

…) (#74925) changes hard coded 10k page size limit to 65k(default for `search.max_bucket`) Closes #57719

hendrikmuhs pushed a commit that referenced this pull request Jul 6, 2021

[Transform] re-enable page size rest test after backport #74651 (#74927)

23a0c87

re-enable test after backport of #74651

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

joegallo added the >enhancement label Sep 22, 2021

changing hard coded 10k page size limit to 65k #74651

changing hard coded 10k page size limit to 65k #74651

Uh oh!

Conversation

Esduard commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benwtrent commented Jun 28, 2021

Uh oh!

elasticmachine commented Jun 28, 2021

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

Esduard Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

Esduard Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

Esduard commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

Esduard commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

Esduard commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jun 30, 2021

Uh oh!

hendrikmuhs commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Esduard commented Jun 28, 2021 •

edited

Loading

hendrikmuhs Jun 29, 2021 •

edited

Loading

hendrikmuhs Jun 29, 2021 •

edited

Loading

hendrikmuhs commented Jun 30, 2021 •

edited

Loading