-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Calculate results and model snapshot retention using latest bucket timestamps #51061
Changes from 6 commits
7c0161a
7ca8db9
f620d6b
6c56dda
11fa81b
aee2d13
498f5b0
580c517
e7724b9
e5026ee
8bd4f99
542869a
c553a42
7569a2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -57,15 +57,15 @@ public void setUpData() throws IOException { | |||||
.setMapping("time", "type=date,format=epoch_millis") | ||||||
.get(); | ||||||
|
||||||
// We are going to create data for last 2 days | ||||||
long nowMillis = System.currentTimeMillis(); | ||||||
// We are going to create 2 days of data starting 24 hrs ago | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
long lastestBucketTime = System.currentTimeMillis() - TimeValue.timeValueHours(1).millis(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: |
||||||
int totalBuckets = 3 * 24; | ||||||
int normalRate = 10; | ||||||
int anomalousRate = 100; | ||||||
int anomalousBucket = 30; | ||||||
BulkRequestBuilder bulkRequestBuilder = client().prepareBulk(); | ||||||
for (int bucket = 0; bucket < totalBuckets; bucket++) { | ||||||
long timestamp = nowMillis - TimeValue.timeValueHours(totalBuckets - bucket).getMillis(); | ||||||
long timestamp = lastestBucketTime - TimeValue.timeValueHours(totalBuckets - bucket).getMillis(); | ||||||
int bucketRate = bucket == anomalousBucket ? anomalousRate : normalRate; | ||||||
for (int point = 0; point < bucketRate; point++) { | ||||||
IndexRequest indexRequest = new IndexRequest(DATA_INDEX); | ||||||
|
@@ -208,7 +208,7 @@ public void testDeleteExpiredData() throws Exception { | |||||
assertThat(getModelSnapshots("no-retention").size(), equalTo(2)); | ||||||
|
||||||
List<Bucket> buckets = getBuckets("results-retention"); | ||||||
assertThat(buckets.size(), is(lessThanOrEqualTo(24))); | ||||||
assertThat(buckets.size(), is(lessThanOrEqualTo(25))); | ||||||
assertThat(buckets.size(), is(greaterThanOrEqualTo(22))); | ||||||
assertThat(buckets.get(0).getTimestamp().getTime(), greaterThanOrEqualTo(oneDayAgo)); | ||||||
assertThat(getRecords("results-retention").size(), equalTo(0)); | ||||||
|
@@ -223,7 +223,7 @@ public void testDeleteExpiredData() throws Exception { | |||||
assertThat(getModelSnapshots("snapshots-retention-with-retain").size(), equalTo(2)); | ||||||
|
||||||
buckets = getBuckets("results-and-snapshots-retention"); | ||||||
assertThat(buckets.size(), is(lessThanOrEqualTo(24))); | ||||||
assertThat(buckets.size(), is(lessThanOrEqualTo(25))); | ||||||
assertThat(buckets.size(), is(greaterThanOrEqualTo(22))); | ||||||
assertThat(buckets.get(0).getTimestamp().getTime(), greaterThanOrEqualTo(oneDayAgo)); | ||||||
assertThat(getRecords("results-and-snapshots-retention").size(), equalTo(0)); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,19 +68,29 @@ private void removeData(WrappedBatchedJobsIterator jobIterator, ActionListener<B | |
removeData(jobIterator, listener, isTimedOutSupplier); | ||
return; | ||
} | ||
long cutoffEpochMs = calcCutoffEpochMs(retentionDays); | ||
removeDataBefore(job, cutoffEpochMs, | ||
ActionListener.wrap(response -> removeData(jobIterator, listener, isTimedOutSupplier), listener::onFailure)); | ||
|
||
calcCutoffEpochMs(job.getId(), retentionDays, ActionListener.wrap( | ||
cutoffEpochMs -> { | ||
if (cutoffEpochMs == null) { | ||
removeData(jobIterator, listener, isTimedOutSupplier); | ||
} else { | ||
removeDataBefore(job, cutoffEpochMs, ActionListener.wrap( | ||
response -> removeData(jobIterator, listener, isTimedOutSupplier), | ||
listener::onFailure)); | ||
} | ||
}, | ||
listener::onFailure | ||
)); | ||
} | ||
|
||
private WrappedBatchedJobsIterator newJobIterator() { | ||
BatchedJobsIterator jobsIterator = new BatchedJobsIterator(client, AnomalyDetectorsIndex.configIndexName()); | ||
return new WrappedBatchedJobsIterator(jobsIterator); | ||
} | ||
|
||
private long calcCutoffEpochMs(long retentionDays) { | ||
void calcCutoffEpochMs(String jobId, long retentionDays, ActionListener<Long> listener) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other methods that extending classes are expected to override are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method is package-private for testing otherwise it is very difficult to test and can only be done indirectly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that the base class is package private, only classes in the same package can implement the abstract method whether the method is protected or package private. The only difference is I could derive a new class from one of the package's public non-abstract classes and reimplement |
||
long nowEpochMs = Instant.now(Clock.systemDefaultZone()).toEpochMilli(); | ||
return nowEpochMs - new TimeValue(retentionDays, TimeUnit.DAYS).getMillis(); | ||
listener.onResponse(nowEpochMs - new TimeValue(retentionDays, TimeUnit.DAYS).getMillis()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this method be abstract instead of providing a default based on wall clock time? It seems that now we've made deletion of model snapshots and results relative to latest bucket time rather than wall clock time we should do that for all job related documents that have a timestamp. So having a default implementation of this method that uses wall clock time just seems like a way that we'll introduce a bug by accidentally deleting some other type of document based on wall clock time. |
||
|
||
protected abstract Long getRetentionDays(Job job); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,10 +82,10 @@ static SearchResponse createSearchResponse(List<? extends ToXContent> toXContent | |
static void givenJobs(Client client, List<Job> jobs) throws IOException { | ||
SearchResponse response = AbstractExpiredJobDataRemoverTests.createSearchResponse(jobs); | ||
|
||
doAnswer(invocationOnMock -> { | ||
ActionListener<SearchResponse> listener = (ActionListener<SearchResponse>) invocationOnMock.getArguments()[2]; | ||
listener.onResponse(response); | ||
return null; | ||
doAnswer(invocationOnMock -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this is indented more than the line above. |
||
ActionListener<SearchResponse> listener = (ActionListener<SearchResponse>) invocationOnMock.getArguments()[2]; | ||
listener.onResponse(response); | ||
return null; | ||
}).when(client).execute(eq(SearchAction.INSTANCE), any(), any()); | ||
} | ||
|
||
|
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.
@szabosteve can you look over these docs changes please. Do they make sense?
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.
@davidkyle Sorry, I haven't noticed this one earlier.