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

support return AD job when get detector #50

Merged

Conversation

ylwu-amzn
Copy link
Contributor

@ylwu-amzn ylwu-amzn commented Feb 29, 2020

  • Add job disabled time
  • Support return AD job when get detector if set ?job=true
  • Tune start job logic:
    1). If job not exists, create new job.
    2). If job exists: a). if job state is enabled, return warning message; b). if job state is disabled, enable job.
  • Tune stop job logic:
    1). If job not exists, return error message
    2). If job exists: a).if job state is disabled, return warning message; b).if job state is enabled, disable job and stop AD model.
  • Tune delete detector logic:
    1). If no AD job index or AD job not found or AD job is disabled, delete detector directly
    2). If AD job state is enabled, return warning message, not delete detector

channel.sendResponse(new BytesRestResponse(response.status(), response.toXContent(channel.newErrorBuilder(), EMPTY_PARAMS)));
}
if (function != null) {
function.execute();
Copy link
Member

Choose a reason for hiding this comment

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

so function does not send back response using channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function will send back response.
Check line 287 of file IndexAnomalyDetectorJobActionHandler

stopAdDetectorListener(channel, detectorId)

Copy link
Member

Choose a reason for hiding this comment

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

Any way to enforce sending back response? It is easy for function implementor to forget to add a channel.sendResponse.

Copy link
Contributor Author

@ylwu-amzn ylwu-amzn Mar 4, 2020

Choose a reason for hiding this comment

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

This is a private function and it doesn't implicitly require to send response to channel here. Depends on how developer implements the logic.

Copy link
Member

Choose a reason for hiding this comment

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

AnomalyDetectorFunction is too general and can be anything. We want to make an interface specific for a specific thing. I would suggest:

  1. rename AnomalyDetectorFunction to ListenerWithChannel or sth that is more specific
  2. make execute take channel as a parameter
  3. add a comment before channel indicating user should send channel response.

Copy link
Contributor Author

@ylwu-amzn ylwu-amzn Mar 6, 2020

Choose a reason for hiding this comment

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

We can do that. How about we split such refactoring work into another issue. So we can make this PR more focusing. Thanks for the suggestion.

@ylwu-amzn ylwu-amzn merged commit 6b265d4 into opendistro-for-elasticsearch:development Mar 6, 2020
kaituo pushed a commit to kaituo/anomaly-detection that referenced this pull request Mar 6, 2020
)

* support return AD job when get detector
* handle null index response
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants