Add valuesField in PercentilesAggregationBuilder streamInput constructor

[sbcd90]

Signed-off-by: Subhobrata Dey [email protected]

Description

Ensures that valuesField is correctly populated in PercentilesAggregationBuilder even when the serialized query is passed as argument to the constructor.

Issues Resolved

opensearch-project/alerting#306

Check List

[X] All tests pass
[X] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

Can one of the admins verify this patch?

[opensearch-ci-bot]

❌   Gradle Check failure 4c46efcc559f3a5551e0745c4aaee6692504efb1
Log 2920

Reports 2920

[dblock]

How did you find this? This needs a test, please.

[sbcd90]

@dblock I'll add a test case which simulates this issue: opensearch-project/alerting#306
The other constructors already take care of populating valuesField correctly in AbstractPercentilesAggregationBuilder.

[sbcd90]

Hi @dblock , added the test case which simulates the issue. Without the fix, test case fails.

[opensearch-ci-bot]

❌   Gradle Check failure be889987787a4700a2db9b96dfab622e21034a07
Log 2953

Reports 2953

[opensearch-ci-bot]

✅   Gradle Check success 624a3ca4c5257cd40d44c5d214fb05dea29c7e7c
Log 2955

Reports 2955

[andrross]

It's a bit weird that these child classes have to know to assign this field or else things don't work correctly. I believe the two child classes you modified here are the only places this constructor is called? What do you think about modifying the parent constructor to take a StreamInput in along with a ParseField valuesField so that the class contract makes it clear that field needs to be defined by the implementer? Then you just call super(in, VALUES_FIELD) here.

[sbcd90]

sure @andrross. this is the alternative solution i thought about. i'll make this change & push it now.

[opensearch-ci-bot]

❌   Gradle Check failure 09fb3facd923103c9a3b6088ac31f15a57af71d7
Log 2983

Reports 2983

[andrross]

REPRODUCE WITH: ./gradlew ':qa:full-cluster-restart:v1.3.0#oldClusterTest' --tests "org.opensearch.upgrades.FullClusterRestartIT.testEnableSoftDeletesOnRestore" -Dtests.seed=4F389D583F9DD654 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ar-LY -Dtests.timezone=GB-Eire -Druntime.java=17

org.opensearch.upgrades.FullClusterRestartIT > testEnableSoftDeletesOnRestore FAILED
    org.opensearch.client.WarningFailureException: method [PUT], host [http://127.0.0.1:40315], URI [/testenablesoftdeletesonrestore], status line [HTTP/1.1 200 OK]
    {"acknowledged":true,"shards_acknowledged":true,"index":"testenablesoftdeletesonrestore"}
        at __randomizedtesting.SeedInfo.seed([4F389D583F9DD654:7B6759F1E30E686F]:0)
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:346)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:320)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:295)
        at app//org.opensearch.test.rest.OpenSearchRestTestCase.createIndex(OpenSearchRestTestCase.java:976)
        at app//org.opensearch.test.rest.OpenSearchRestTestCase.createIndex(OpenSearchRestTestCase.java:956)
        at app//org.opensearch.test.rest.OpenSearchRestTestCase.createIndex(OpenSearchRestTestCase.java:952)
        at app//org.opensearch.upgrades.FullClusterRestartIT.testEnableSoftDeletesOnRestore(FullClusterRestartIT.java:1551)

REPRODUCE WITH: ./gradlew ':qa:full-cluster-restart:v1.3.0#oldClusterTest' --tests "org.opensearch.upgrades.FullClusterRestartIT.testResize" -Dtests.seed=4F389D583F9DD654 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ar-LY -Dtests.timezone=GB-Eire -Druntime.java=17

org.opensearch.upgrades.FullClusterRestartIT > testResize FAILED
    org.opensearch.client.WarningFailureException: method [PUT], host [http://127.0.0.1:45495], URI [/testresize], status line [HTTP/1.1 200 OK]
    {"acknowledged":true,"shards_acknowledged":true,"index":"testresize"}
        at __randomizedtesting.SeedInfo.seed([4F389D583F9DD654:F84787AC20DB4C6F]:0)
        at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:346)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:320)
        at app//org.opensearch.client.RestClient.performRequest(RestClient.java:295)
        at app//org.opensearch.test.rest.OpenSearchRestTestCase.createIndex(OpenSearchRestTestCase.java:976)
        at app//org.opensearch.test.rest.OpenSearchRestTestCase.createIndex(OpenSearchRestTestCase.java:956)
        at app//org.opensearch.upgrades.FullClusterRestartIT.testResize(FullClusterRestartIT.java:1387)

[andrross]

start gradle check

[opensearch-ci-bot]

❌   Gradle Check failure 09fb3facd923103c9a3b6088ac31f15a57af71d7
Log 2990

Reports 2990

[sbcd90]

Hi @andrross , are these test failures related to the code changes? please let me know.

[andrross]

Can you rebase this PRs? I think there are some fixes that you need to merge in to get the build to pass.

[opensearch-ci-bot]

✅   Gradle Check success 3f466f2
Log 3060

Reports 3060

[mhoydis13]

Do we suppose this patch also resolves issue 1680? opensearch-project/security#1680

UPDATE: I can confirm this does indeed resolve issue 1680.