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

Write detection code path in callbacks #48

Merged
merged 6 commits into from
Mar 9, 2020

Conversation

kaituo
Copy link
Member

@kaituo kaituo commented Feb 26, 2020

Previously, we actively wait for rcf and threshold model requests to return in transport thread. The active wait blocks transport thread. To increase concurrency and improve performance, we change detection code path to use callbacks.

Testing done:

  • all unit and integration tests pass
  • run end-to-end testing to verify basic workflow works including creating detector and generate anomaly score

@kaituo kaituo requested review from wnbts and ylwu-amzn February 26, 2020 04:17
@kaituo kaituo assigned ylwu-amzn and kaituo and unassigned ylwu-amzn Feb 26, 2020
@kaituo kaituo requested a review from zhanghg08 February 27, 2020 03:40
} else {
LOG.warn(NULL_RESPONSE + " {} for {}", modelID, rcfNodeID);
}
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

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

Catch exception and log it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

added

this.endTime = endTime;
this.nodeCount = nodeCount;
this.responseCount = responseCount;
this.adID = adID;
}

@Override
public void onResponse(RCFResultResponse response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

onResponse will be called multiple times if send request to multiple nodes?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

ylwu-amzn and others added 3 commits March 5, 2020 20:53
* add AD job on top of JobScheduler

* release job lock when job finish or fail

* upgrade jobscheduler to 1.4
)

* support return AD job when get detector
* handle null index response
Copy link
Contributor

@ylwu-amzn ylwu-amzn left a 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 change! Please merge before push.

}

@Test
public void testRunJobWithLocakDuration() throws InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Member Author

Choose a reason for hiding this comment

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

That’s actually from rebased code. not the code meant to be reviewed. I fixed it.

Copy link
Contributor

@zhanghg08 zhanghg08 left a comment

Choose a reason for hiding this comment

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

LGTM

@kaituo kaituo merged commit e2f7208 into opendistro-for-elasticsearch:development Mar 9, 2020
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