-
Notifications
You must be signed in to change notification settings - Fork 80
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we don't need to have doc types such as ScheduledJob.SCHEDULED_JOB_TYPE
and
AlertIndices.MAPPING_TYPE
. Can we remove them now?
Just a side note: Do not create branches for pull request on the main repository. Use forks to create pull requests. |
alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/alerts/AlertMover.kt
Outdated
Show resolved
Hide resolved
...lin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestAcknowledgeAlertAction.kt
Outdated
Show resolved
Hide resolved
...otlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestExecuteMonitorAction.kt
Outdated
Show resolved
Hide resolved
alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/TestHelpers.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestGetMonitorAction.kt
Show resolved
Hide resolved
...lin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestIndexDestinationAction.kt
Show resolved
Hide resolved
Thanks will do that for the next PR. |
+1 to using forks for all PRs. Thanks @lucaswin-amzn. |
.routing(monitorId) | ||
.source(Alert.parse(alertContentParser(hit.sourceRef), hit.id, hit.version) | ||
.copy(state = Alert.State.DELETED) | ||
.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS)) | ||
.version(hit.version) | ||
.versionType(VersionType.EXTERNAL_GTE) | ||
.setIfSeqNo(hit.seqNo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 98 to 104 in 10d51ef
IndexRequest(AlertIndices.HISTORY_WRITE_INDEX) | |
.routing(monitorId) | |
.source(Alert.parse(alertContentParser(hit.sourceRef), hit.id, hit.version) | |
.copy(state = Alert.State.DELETED) | |
.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS)) | |
.setIfSeqNo(hit.seqNo) | |
.setIfPrimaryTerm(hit.primaryTerm) |
This doesn't seem correct - how will the seqNo
and primaryTerm
be retained when moving from the active index to the history index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you are asking. We were keeping the version prior, which is still supported since we are using an external versioning system, but since versions are going away, we might want to keep the document the same as a direct copy from the ACTIVE
state to DELETED
state between the indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sequence numbers and primary terms are specific to an index. They cannot be carried over from the hit
in the active index to the copy in the history index. To avoid stale writes on the alert in the history index we need to continue to use external versioning which is still supported in ES 7: https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html#_internal_versioning_is_no_longer_supported_for_optimistic_concurrency_control
.../kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestIndexMonitorAction.kt
Show resolved
Hide resolved
...lin/com/amazon/opendistroforelasticsearch/alerting/resthandler/RestAcknowledgeAlertAction.kt
Outdated
Show resolved
Hide resolved
alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/alerts/AlertMover.kt
Outdated
Show resolved
Hide resolved
…xing a new document these values need to be 0, but since the document exists in the active alerts index seqNo and primaryTerm have already been set to a value greater than 0.
@jinsoor-amzn The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks for the changes.
My mistake @jinsoor-amzn, It was removed from |
b86bf66
Issue: #39
Description of changes: Update plugin to work with Elasticsearch 7.0
Breaking changes in 7.0
Build output:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.