-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Converting indexPattern to indexList is now async #5173
Changes from 1 commit
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 |
---|---|---|
|
@@ -21,6 +21,7 @@ define(function (require) { | |
this._maxSegments = config.get('courier:maxSegmentCount'); | ||
this._hitsReceived = 0; | ||
this._direction = 'desc'; | ||
this._queueCreated = false; | ||
this._handle = new SegmentedHandle(this); | ||
|
||
// prevent the source from changing between requests, | ||
|
@@ -33,9 +34,13 @@ define(function (require) { | |
*********/ | ||
|
||
SegmentedReq.prototype.start = function () { | ||
var self = this; | ||
|
||
this._complete = []; | ||
this._active = null; | ||
this._segments = []; | ||
this._all = []; | ||
this._queue = []; | ||
|
||
this._mergedResp = { | ||
took: 0, | ||
|
@@ -49,11 +54,13 @@ define(function (require) { | |
// give the request consumer a chance to receive each segment and set | ||
// parameters via the handle | ||
if (_.isFunction(this._initFn)) this._initFn(this._handle); | ||
this._createQueue(); | ||
this._all = this._queue.slice(0); | ||
this._createQueue().then(function (queue) { | ||
self._all = queue.slice(0); | ||
self._queueCreated = true; | ||
|
||
// Send the initial fetch status | ||
this._reportStatus(); | ||
// Send the initial fetch status | ||
self._reportStatus(); | ||
}); | ||
|
||
return SearchReq.prototype.start.call(this); | ||
}; | ||
|
@@ -100,7 +107,7 @@ define(function (require) { | |
}; | ||
|
||
SegmentedReq.prototype.isIncomplete = function () { | ||
return this._queue.length > 0; | ||
return !this._queueCreated || this._queue.length > 0; | ||
}; | ||
|
||
SegmentedReq.prototype.clone = function () { | ||
|
@@ -157,22 +164,27 @@ define(function (require) { | |
}; | ||
|
||
SegmentedReq.prototype._createQueue = function () { | ||
var self = this; | ||
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. I feel like we should be setting 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. I agree that we should flag the queue as created inside createQueue itself. Good catch. I still think it makes sense to default _queueCreated to false during instantiation though since it can technically be leveraged (via reportStatus) prior to start() being invoked. 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. 👍 |
||
var timeBounds = timefilter.getBounds(); | ||
var indexPattern = this.source.get('index'); | ||
var queue = indexPattern.toIndexList(timeBounds.min, timeBounds.max); | ||
|
||
if (!_.isArray(queue)) queue = [queue]; | ||
if (this._direction === 'desc') queue = queue.reverse(); | ||
return indexPattern.toIndexList(timeBounds.min, timeBounds.max) | ||
.then(function (queue) { | ||
if (!_.isArray(queue)) queue = [queue]; | ||
if (self._direction === 'desc') queue = queue.reverse(); | ||
|
||
self._queue = queue; | ||
|
||
return (this._queue = queue); | ||
return queue; | ||
}); | ||
}; | ||
|
||
SegmentedReq.prototype._reportStatus = function () { | ||
return this._handle.emit('status', { | ||
total: this._all.length, | ||
complete: this._complete.length, | ||
remaining: this._queue.length, | ||
hitCount: this._mergedResp.hits.hits.length | ||
total: this._queueCreated ? this._all.length : NaN, | ||
complete: this._queueCreated ? this._complete.length : NaN, | ||
remaining: this._queueCreated ? this._queue.length : NaN, | ||
hitCount: this._queueCreated ? this._mergedResp.hits.hits.length : NaN | ||
}); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ define(function (require) { | |
|
||
require('ui/modules') | ||
.get('kibana') | ||
.directive('validateQuery', function (es, $compile, timefilter, kbnIndex, debounce, Private) { | ||
.directive('validateQuery', function (es, $compile, timefilter, kbnIndex, debounce, Promise, Private) { | ||
var fromUser = Private(require('ui/validate_query/lib/from_user')); | ||
var toUser = require('ui/validate_query/lib/to_user'); | ||
|
||
|
@@ -32,40 +32,37 @@ define(function (require) { | |
}; | ||
|
||
function validator(query) { | ||
var index; | ||
var type; | ||
if (request.abort) request.abort(); | ||
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. In order for request.abort to exist it needs to be the exact promise that was returned by the es client. These changes set request on line 39 instead, so 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. Hmm... I'm not sure if this ever worked then. The promise that was set on This is what existed before: So If that was indeed a bug already, this PR has a bit of urgency to it since it's needed for other PRs, so I'd rather address the bug in a subsequent PR. 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. Agreed |
||
|
||
if ($scope.queryInput) { | ||
useSearchSource(); | ||
} else { | ||
useDefaults(); | ||
} | ||
var prepare = $scope.queryInput ? useSearchSource : useDefaults; | ||
|
||
return sendRequest(); | ||
request = prepare().then(sendRequest); | ||
|
||
function useSearchSource() { | ||
var pattern = $scope.queryInput.get('index'); | ||
if (!pattern) return useDefaults(); | ||
|
||
if (_.isString(pattern)) { | ||
index = pattern; | ||
} else if (_.isFunction(pattern.toIndexList)) { | ||
index = pattern.toIndexList(); | ||
return Promise.resolve({ index: pattern }); | ||
} else if (_.isFunction(_.get(pattern, 'toIndexList'))) { | ||
return pattern.toIndexList().then(function (indexList) { | ||
return { index: indexList }; | ||
}); | ||
} else { | ||
useDefaults(); | ||
return useDefaults(); | ||
} | ||
} | ||
|
||
function useDefaults() { | ||
index = kbnIndex; | ||
type = '__kibanaQueryValidator'; | ||
return Promise.resolve({ | ||
index: kbnIndex, | ||
type: '__kibanaQueryValidator' | ||
}); | ||
} | ||
|
||
function sendRequest() { | ||
request = es.indices.validateQuery({ | ||
index: index, | ||
type: type, | ||
function sendRequest(config) { | ||
return es.indices.validateQuery({ | ||
index: config.index, | ||
type: config.type, | ||
explain: true, | ||
ignoreUnavailable: true, | ||
body: { | ||
|
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.
might read better as
this._queueCreated &&
. I missed the!
at first and thought this was backwardsThere 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.
That wouldn't be the same conditional logic though: this is only negating the first half of the condition.