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

Added function for request recreation that considers the writeable re… #303

Conversation

stevanbz
Copy link
Contributor

@stevanbz stevanbz commented Nov 1, 2022

…gistry used for parsing the aggregations

Added unit tests that checks if the bucket level monitor requests are created and ser/de correctly

Signed-off-by: Stevan Buzejic stevan.buzejic@htecgroup.com

Description

This PR enables serialization/de-serialization of bucket level monitors.
Adds two new methods - recreateObject which is responsible to re-create objects by using the namedWriteableRegistry
and indexMonitor - which calls recreateObject taking into the account namedWriteableRegistry

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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.

…gistry used for parsing the aggregations

Added unit tests that checks if the bucket level monitor requests are created and ser/de correctly

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
@stevanbz stevanbz requested a review from a team November 1, 2022 16:44
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2022

Codecov Report

Merging #303 (d26a85f) into main (f9ae96c) will increase coverage by 0.04%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main     #303      +/-   ##
============================================
+ Coverage     73.32%   73.37%   +0.04%     
- Complexity      696      697       +1     
============================================
  Files           110      110              
  Lines          4563     4564       +1     
  Branches        605      605              
============================================
+ Hits           3346     3349       +3     
+ Misses          966      965       -1     
+ Partials        251      250       -1     
Impacted Files Coverage Δ
...search/commons/alerting/AlertingPluginInterface.kt 46.15% <0.00%> (ø)
...n/org/opensearch/commons/utils/TransportHelpers.kt 100.00% <ø> (ø)
...ensearch/commons/rest/SecureRestClientBuilder.java 52.77% <0.00%> (+0.44%) ⬆️
...n/org/opensearch/commons/alerting/model/Monitor.kt 81.52% <0.00%> (+0.54%) ⬆️
...n/org/opensearch/commons/alerting/model/Trigger.kt 85.00% <0.00%> (+5.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

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

Thanks for catching this issue and fixing it Stevan. Left a couple minor comments

@@ -50,6 +51,31 @@ object AlertingPluginInterface {
}
)
}
/**
Copy link
Member

Choose a reason for hiding this comment

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

let's not overload index monitor method. Let's change the original method's signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure. Tnx

randomBucketLevelMonitor()
)

val recreatedObject = recreateObject(req, NamedWriteableRegistry(SearchModule(Settings.EMPTY, emptyList()).namedWriteables)) { IndexMonitorRequest(it) }
Copy link
Member

Choose a reason for hiding this comment

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

let's have a test method where recreate object method WITHOUT NamedWriteableRegistry fails and in the same method let's verify that recreate Object WITH NamedWriteableRegistry passes using an actual aggregation in IndexMonitorRequest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me try to do that

…consider scenario when recreation of aggregation monitor fails

Signed-off-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
@eirsep eirsep merged commit 6a169f7 into opensearch-project:main Nov 1, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 1, 2022
#303)

* Added function for request recreation that considers the writeable registry used for parsing the aggregations

Co-authored-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 6a169f7)
eirsep pushed a commit that referenced this pull request Nov 1, 2022
#303) (#304)

* Added function for request recreation that considers the writeable registry used for parsing the aggregations

Co-authored-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 6a169f7)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 2, 2022
#303)

* Added function for request recreation that considers the writeable registry used for parsing the aggregations

Co-authored-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 6a169f7)
opensearch-trigger-bot bot added a commit that referenced this pull request Nov 2, 2022
#303) (#304)

* Added function for request recreation that considers the writeable registry used for parsing the aggregations

Co-authored-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 6a169f7)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
(cherry picked from commit 2b2f242)
sbcd90 pushed a commit that referenced this pull request Nov 2, 2022
#303) (#311)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
wuychn pushed a commit to ochprince/common-utils that referenced this pull request Mar 16, 2023
opensearch-project#303) (opensearch-project#304)

* Added function for request recreation that considers the writeable registry used for parsing the aggregations

Co-authored-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 6a169f7)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
AWSHurneyt pushed a commit to AWSHurneyt/common-utils that referenced this pull request Apr 12, 2024
opensearch-project#303) (opensearch-project#304)

* Added function for request recreation that considers the writeable registry used for parsing the aggregations

Co-authored-by: Stevan Buzejic <stevan.buzejic@htecgroup.com>
(cherry picked from commit 6a169f7)

Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants