Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML][Data Frames] Fix null aggregation handling in indexer #45061

Merged
merged 6 commits into from
Aug 6, 2019
Merged

[ML][Data Frames] Fix null aggregation handling in indexer #45061

merged 6 commits into from
Aug 6, 2019

Conversation

benwtrent
Copy link
Member

Since we are more lenient now when expanding index patterns, it could be that there are no open concrete indices contained within the pattern. When this is the case SearchResponse#getAggregations() returns null.

This adds a null check for that method, audits that possibly all the underlying indices are closed/deleted, and returns that the current checkpoint is done.

closes #44906

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@benwtrent benwtrent force-pushed the bug/ml-df-fix-npe-when-aggs-are-null branch from 5be3009 to f4fd537 Compare July 31, 2019 20:07
benwtrent
benwtrent commented Jul 31, 2019
View reviewed changes
.../data-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameIndexer.java
"Data frame transform unexpectedly stopped seeing data. " +
"Please verify that these indices exist and are open [" +
Strings.arrayToCommaDelimitedString(getConfig().getSource().getIndex()) +
"].");
Copy link
Member Author

Choose a reason for hiding this comment

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

I waffled a bit on how to word this. From my understanding, aggregations should only ever be null here iff the underlying indices are all closed/deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's only null if source became empty.

With the given wording, it sounds more like a warning. So I would either change the log/audit level or the wording.

@benwtrent
Copy link
Member Author

run elasticsearch-ci/packaging-sample

hendrikmuhs
hendrikmuhs reviewed Aug 2, 2019
View reviewed changes
Copy link
Contributor

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

What about other places in the code? My gut feeling about #44906: it's more than this, see the dev notes about _preview and deferred validation.

.../data-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameIndexer.java
"Data frame transform unexpectedly stopped seeing data. " +
"Please verify that these indices exist and are open [" +
Strings.arrayToCommaDelimitedString(getConfig().getSource().getIndex()) +
"].");
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's only null if source became empty.

With the given wording, it sounds more like a warning. So I would either change the log/audit level or the wording.

@benwtrent
Copy link
Member Author

@hendrikmuhs we already validate for concrete index existence on _start and on _preview.
Preview: https://github.com/elastic/elasticsearch/blob/204ed8509a4687f8e5067f825a14fe822aad46d0/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/action/TransportPreviewDataFrameTransformAction.java#L94..L103

Start: https://github.com/elastic/elasticsearch/blob/204ed8509a4687f8e5067f825a14fe822aad46d0/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/action/TransportStartDataFrameTransformAction.java#L184..L185

@benwtrent
Copy link
Member Author

@elasticmachine update branch

droberts195
droberts195 reviewed Aug 6, 2019
View reviewed changes
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

I don't think we need the words "Concrete" or "Underlying" in the messages, but apart from that I'm happy with this change

.../java/org/elasticsearch/xpack/dataframe/action/TransportPreviewDataFrameTransformAction.java Outdated Show resolved Hide resolved
.../data-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameIndexer.java Outdated Show resolved Hide resolved
.../data-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameIndexer.java Outdated Show resolved Hide resolved
@benwtrent
Copy link
Member Author

@elasticmachine update branch

hendrikmuhs
hendrikmuhs approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

I am not sure about the logging/audit w.r.t. a PR I am working on, but this can be revisited as a follow up.

droberts195
droberts195 approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@benwtrent
Copy link
Member Author

run elasticsearch-ci/2

@benwtrent benwtrent merged commit b78053c into elastic:master Aug 6, 2019
@benwtrent benwtrent deleted the bug/ml-df-fix-npe-when-aggs-are-null branch August 6, 2019 19:55
@benwtrent benwtrent mentioned this pull request Aug 6, 2019
Merged
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Aug 6, 2019
@benwtrent
[ML][Data Frames] Fix null aggregation handling in indexer (elastic#4…
53b3066 
…5061)

* [ML][Data Frames] Fix null aggregation handling in indexer

* addressing PR comments

* adjusting error messages
@benwtrent benwtrent mentioned this pull request Aug 6, 2019
Merged
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Aug 6, 2019
@benwtrent
[ML][Data Frames] Fix null aggregation handling in indexer (elastic#4…
6711971 
…5061)

* [ML][Data Frames] Fix null aggregation handling in indexer

* addressing PR comments

* adjusting error messages
benwtrent added a commit that referenced this pull request Aug 7, 2019
@benwtrent
[ML][Data Frames] Fix null aggregation handling in indexer (#45061) (#…
be911e6 
…45257)

* [ML][Data Frames] Fix null aggregation handling in indexer

* addressing PR comments

* adjusting error messages
benwtrent added a commit that referenced this pull request Aug 7, 2019
@benwtrent
[ML][Data Frames] Fix null aggregation handling in indexer (#45061) (#…
40266e5 
…45258)

* [ML][Data Frames] Fix null aggregation handling in indexer

* addressing PR comments

* adjusting error messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hendrikmuhs hendrikmuhs hendrikmuhs approved these changes

@droberts195 droberts195 droberts195 approved these changes

Assignees
No one assigned
Labels
>bug :ml/Transform Transform v7.3.1 v7.4.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ML-DataFrame] NPE if no source index available / revisit empty set of source indexes
5 participants
@benwtrent @elasticmachine @hendrikmuhs @droberts195 @jakelandis