Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add action to /_acknowledge/alerts api #236

Merged
merged 2 commits into from
Aug 24, 2020
Merged

Add action to /_acknowledge/alerts api #236

merged 2 commits into from
Aug 24, 2020

Conversation

skkosuri-amzn
Copy link
Contributor

Issue #, if available:
#232
Description of changes:
#232

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #236 into master will increase coverage by 0.81%.
The diff coverage is 90.24%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #236      +/-   ##
============================================
+ Coverage     76.96%   77.78%   +0.81%     
- Complexity      157      164       +7     
============================================
  Files            75       79       +4     
  Lines          2809     3043     +234     
  Branches        430      447      +17     
============================================
+ Hits           2162     2367     +205     
- Misses          428      440      +12     
- Partials        219      236      +17     
Impacted Files Coverage Δ Complexity Δ
...icsearch/alerting/action/AcknowledgeAlertAction.kt 66.66% <66.66%> (ø) 0.00 <0.00> (?)
...stroforelasticsearch/alerting/alerts/AlertError.kt 80.95% <85.71%> (+0.95%) 0.00 <0.00> (ø)
...sticsearch/alerting/model/ActionExecutionResult.kt 87.09% <87.50%> (+0.14%) 0.00 <0.00> (ø)
...search/alerting/action/AcknowledgeAlertResponse.kt 88.88% <88.88%> (ø) 0.00 <0.00> (?)
...rting/transport/TransportAcknowledgeAlertAction.kt 88.88% <88.88%> (ø) 0.00 <0.00> (?)
...opendistroforelasticsearch/alerting/model/Alert.kt 89.36% <91.66%> (+0.79%) 0.00 <0.00> (ø)
...ndistroforelasticsearch/alerting/AlertingPlugin.kt 89.39% <100.00%> (+0.16%) 0.00 <0.00> (ø)
...csearch/alerting/action/AcknowledgeAlertRequest.kt 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...alerting/resthandler/RestAcknowledgeAlertAction.kt 79.16% <100.00%> (-11.85%) 0.00 <0.00> (ø)
...endistroforelasticsearch/alerting/model/Monitor.kt 86.95% <0.00%> (-3.75%) 0.00% <0.00%> (ø%)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 263f55d...580f612. Read the comment docs.

constructor(sin: StreamInput) : super() {
this.acknowledged = Collections.unmodifiableList(sin.readList(::Alert))
this.failed = Collections.unmodifiableList(sin.readList(::Alert))
this.missing = sin.readStringList()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should this be left as mutable?

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 catch, fixed.

Comment on lines 34 to 35
sin.readInstant(),
sin.readString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Label these with the attribute names to keep the documentation consistent throughout the package and please do so everywhere else in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments

Comment on lines +66 to +74
client.search(searchRequest, object : ActionListener<SearchResponse> {
override fun onResponse(response: SearchResponse) {
onSearchResponse(response)
}

override fun onFailure(t: Exception) {
actionListener.onFailure(t)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why not continue using the wrap function for ActionListener, so it will be like:
client.search(searchRequest, ActionListener<SearchResponse>.wrap(::onSearchResponse, ::onFailure))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started this pattern mainly to customize exception and returned rest status with onFailure. Something like:
actionListener.onFailure(ElasticsearchStatusException("my error msg",RestStatus.INTERNAL_SERVER_ERROR))

@skkosuri-amzn skkosuri-amzn merged commit 07762df into master Aug 24, 2020
@bowenlan-amzn bowenlan-amzn added the enhancement New feature or request label Sep 8, 2020
@qreshi qreshi deleted the ack-api branch December 19, 2020 00:47
tlfeng pushed a commit that referenced this pull request Feb 6, 2021
* Add action to /_acknowledge/alerts api
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants