Skip to content
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

Implement buildEmptyAggregations for MultiTermsAggregator (#7089) #7318

Merged
merged 4 commits into from
Jun 29, 2023

Conversation

austintlee
Copy link
Contributor

Description

The implementation for the 'buildEmptyAggregation' method for MultiTermsAggregator was incomplete and was simply returning null. This change completes the implementation for this method and adds a YAML REST test case for it.

I have another PR that has a unit test for this change. I want to get feedback on that separately from this PR.

Issues Resolved

#7089
#5785

Check List

  • New functionality includes testing.
    • [x ] All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • [x ] Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented Apr 28, 2023

This looks great. Since the unit test detects this exact problem let's roll it in here?

@austintlee
Copy link
Contributor Author

@dblock I can add the unit test commit to this one. What do you think?

@dblock
Copy link
Member

dblock commented May 1, 2023

@dblock I can add the unit test commit to this one. What do you think?

Yep, you should.

Iterate this PR to green. I haven't looked at the failures in the gradle check, could be legit. And thank you!

@austintlee austintlee force-pushed the issue-7089-multi-term-agg branch from ea238f1 to 6e9423a Compare May 2, 2023 00:51
@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

Gradle Check (Jenkins) Run Completed with:

@austintlee
Copy link
Contributor Author

Can I get some help with this? I don't know why Gradle Check failed.

CHANGELOG.md Outdated Show resolved Hide resolved
@reta
Copy link
Collaborator

reta commented May 11, 2023

The Gradle check may fail due to #7470 (#7473 just went into 2.x)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testCancelPrimaryAllocation
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

@andrross andrross force-pushed the issue-7089-multi-term-agg branch from 1fe4cfd to 0075664 Compare June 19, 2023 23:17
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
Update version check and reason

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
@andrross andrross force-pushed the issue-7089-multi-term-agg branch from 0075664 to 14fd215 Compare June 28, 2023 23:27
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication
      1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollCreatedOnReplica
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

@reta reta added v2.5.0 'Issues and PRs related to version v2.5.0' v2.9.0 'Issues and PRs related to version v2.9.0' and removed v2.5.0 'Issues and PRs related to version v2.5.0' labels Jun 29, 2023
@reta reta merged commit 86f955c into opensearch-project:main Jun 29, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 29, 2023
)

* Implement buildEmptyAggregations for MultiTermsAggregator (#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Address Spotless check issue

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Add a unit test for MultiTermsAggregator.buildEmptyAggregations (#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Update changelog

Update version check and reason

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

---------

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
(cherry picked from commit 86f955c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Jun 29, 2023
)

* Implement buildEmptyAggregations for MultiTermsAggregator (#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Address Spotless check issue

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Add a unit test for MultiTermsAggregator.buildEmptyAggregations (#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Update changelog

Update version check and reason

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

---------

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
(cherry picked from commit 86f955c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta pushed a commit that referenced this pull request Jun 29, 2023
) (#8333)

* Implement buildEmptyAggregations for MultiTermsAggregator (#7089)



* Address Spotless check issue



* Add a unit test for MultiTermsAggregator.buildEmptyAggregations (#7089)



* Update changelog

Update version check and reason



---------


(cherry picked from commit 86f955c)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
…-project#7089) (opensearch-project#7318)

* Implement buildEmptyAggregations for MultiTermsAggregator (opensearch-project#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Address Spotless check issue

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Add a unit test for MultiTermsAggregator.buildEmptyAggregations (opensearch-project#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Update changelog

Update version check and reason

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

---------

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…-project#7089) (opensearch-project#7318)

* Implement buildEmptyAggregations for MultiTermsAggregator (opensearch-project#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Address Spotless check issue

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Add a unit test for MultiTermsAggregator.buildEmptyAggregations (opensearch-project#7089)

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

* Update changelog

Update version check and reason

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>

---------

Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch v2.9.0 'Issues and PRs related to version v2.9.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants