Remove Segmented Request #16324
Remove Segmented Request #16324
Conversation
|
jenkins, test this |
💚 Build Succeeded |
There was a problem hiding this comment.
Left a couple of comments inline.
This is awesome! I'm so excited about this. As I was looking through this I noticed that we still have some references to notExpandable. As far as I can tell, notExpandable isn't used anywhere and, although it's not completely related to this PR, it might be nice to go through and remove all remaining references.
| @@ -31,6 +30,12 @@ import { migrateLegacyQuery } from 'ui/utils/migrateLegacyQuery'; | |||
| import { FilterManagerProvider } from 'ui/filter_manager'; | |||
| import { SavedObjectsClientProvider } from 'ui/saved_objects'; | |||
|
|
|||
| const fetchStatuses = { | |||
There was a problem hiding this comment.
Looks like we're using these same constants in the context app. Maybe we could move them to a more central location (maybe here) and use them for both places?
There was a problem hiding this comment.
Hah that's funny, I wrote those statuses without even realizing context had a similar thing, maybe I did it subconsciously. I only fear that these statuses have slightly different meanings in each app because they fetch results in slightly different ways (context uses fetchAsRejectablePromise, discover uses the onResults callback). If we couple them, it might lead to some contention down the road. I've got some thoughts on how to fix #14544 by making context and discover more similar, I'd like to wait till then to combine the statuses if that's ok with you?
There was a problem hiding this comment.
Realized I never responded to this. I thought these were constants that weren't arbitrarily decided on by us. With your explanation I realize they are different, so I think it's fine to punt on this.
| this._initFn = initFn; | ||
|
|
||
| this._desiredSize = null; | ||
| this._maxSegments = config.get('courier:maxSegmentCount'); |
There was a problem hiding this comment.
I guess at this point we should remove this advanced setting, right?
There was a problem hiding this comment.
Crap, I forgot about this setting. I was thinking we could make this change in 6.x since it only affects implementation details. But the fact that we're exposing a setting to tune those details may change things. What do you think @epixa ?
There was a problem hiding this comment.
I just left a comment on the PR as a whole that is sort of related to this. In general, if an advanced setting is truly not doing anything useful anymore, I'm fine with it being removed so long as doing so doesn't break existing kibana installs that may have it set explicitly.
There was a problem hiding this comment.
So, it does still have an effect before this change. It sets a max on the number of requests a search can be segmented into. After this change, searches are no longer broken up into segments, so the setting is useless. I can't think of a situation where segmentation would actually be useful (slow queries maybe, but to me that's a bug in ES or the request itself), but in theory someone might be relying on it for reasons unknown to me at this time.
There was a problem hiding this comment.
In that case, this does seem like a breaking change to me. Regardless of the use case, at some point we felt there was enough of a reason to give people control over their segmenting behavior. Our lack of memory or knowledge on that shouldn't be enough of a reason to just remove it. It seems like we should target master and remove all of this alongside the interval index pattern support itself.
There was a problem hiding this comment.
Went ahead and removed this setting since I'm only targeting 7.0 now.
|
|
|
Have you tested this PR with the old interval-based index patterns? I know we can't create any new ones, but we shouldn't break existing index patterns in 6.x. The new search optimizations in ES only work with wildcard indices, so the interval-based index pattern names wouldn't take advantage of it since those were handled with individual requests to direct indices. |
|
@epixa yeah, interval based index patterns still work, they just run as a single query against all of the matching indices instead of one query per matching index. My thinking was that this is fine since the patterns will still work, we'll just be making the request differently under the hood. But it turns out we give the users some control over tuning that under the hood behavior (see inline comment above), which would suddenly have no effect with this change. |
|
@Bargs I'm not sure if the optimizations on the ES end work with explicit index names. It was my understanding they only affect wildcard patterns. |
|
@Bargs 👍 to your response about |
|
This week I'm going to rebase this PR and make sure it's cleaned up, then I'd appreciate one more look and an lgtm if all's well. We'll just merge this into master since it's a breaking change and then I'll submit my multi-sort enhancement to master as well. |
💔 Build Failed |
Bargs
force-pushed
the
removeSegmentedFetch
branch
from
dc61193 to
8ab99a6
Compare
💚 Build Succeeded |
|
Alright, CI is green again, @lukasolson wanna take another look? |
|
Conflicts again |
Bargs
force-pushed
the
removeSegmentedFetch
branch
from
3fe8086 to
11cb33d
Compare
|
Conflicts resolved. @epixa you wanna take a look at this or should I grab someone else for a second review? |
💚 Build Succeeded |
|
@Bargs I should have time later this week to review it if no one else gets to it in the meantime |
💔 Build Failed |
|
@timroes @ppisljar we really should get this into 7.0. Getting rid of segmented request would dramatically reduce the complexity of Discover but it's a breaking change so it needs to go in a major. I just rebased on the latest master and ran into some issues with vis and inspector changes. If I could get some time with one of you to walk through it I think they should be easy to resolve. |
|
Can we get this updated? And prepared for 7.0? |
|
It's still on my todo list but I'm currently slammed. @timroes I wonder if someone else could pick this up? |
|
Superseded by #33453. |
Segmented Request was only being used in Discover. It was only actually segmenting requests for old interval based index patterns (and not even for these index patterns in Dashboard). Interval based index patterns are deprecated (cannot be created in 6.0) and will be completely removed in 7.0. The fact that interval based patterns got segmented was an implementation detail of Kibana that is no longer necessary now that ES is smart enough to quickly skip shards that won't return results. The segmented request code creates a huge maintenance burden that prevents us from easily implementing highly requested features like multi-field sorting. This also makes the search fetching behavior more consistent between Discover and Dashboard (which actually relies on the doc table making requests), which will make it easier to extract that logic into a reusable service when we want to rewrite the doc table in React and remove its dependence on Courier.
tl;dr let's get this monkey off our back