Skip to content

Expose max_concurrent_shard_requests for _msearch requests#22379

Merged
Bargs merged 2 commits intoelastic:masterfrom
Bargs:addMaxShardRequestsSetting
Sep 11, 2018
Merged

Expose max_concurrent_shard_requests for _msearch requests#22379
Bargs merged 2 commits intoelastic:masterfrom
Bargs:addMaxShardRequestsSetting

Conversation

@Bargs
Copy link
Contributor

@Bargs Bargs commented Aug 24, 2018

Fixes #22206

Allows Kibana users to configure the max_concurrent_shard_requests param used by Kibana when sending _msearch requests. Exposes the config as an advanced setting. By default we won't send the param at all, relying on the ES default instead.

screen shot 2018-08-24 at 1 22 49 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this technically works, I'd like to change it to use the snake case version once the es.js API generation script is re-run to make maxConcurrentShardRequests a first class citizen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so elasticsearch.js will pass arbitrary properties through? I didn't realize that...

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@epixa
Copy link
Contributor

epixa commented Aug 27, 2018

A negative value for this will cause fatal errors to be fired, and I had to navigate by URL back to the management panel to fix the issue. Can we do some validation to ensure it is either zero or a positive integer?

Also, can you add tests?

@s1monw
Copy link

s1monw commented Sep 3, 2018

@Bargs any progress on this?

@Bargs
Copy link
Contributor Author

Bargs commented Sep 4, 2018

Been OOO for the past week, I should be able to make updates based on Court's feedback this week.

@s1monw
Copy link

s1monw commented Sep 10, 2018

@Bargs ping :)

@Bargs
Copy link
Contributor Author

Bargs commented Sep 10, 2018

@epixa which page were you getting fatal errors on? I'm only able to get a regular error that already has a pretty clear message:

screen shot 2018-09-10 at 12 28 57 pm

Validation on the advanced settings page itself would be ideal, but afaik we don't have an existing mechanism for validation on that page and building one would be beyond the scope of this PR IMO.

@epixa
Copy link
Contributor

epixa commented Sep 10, 2018

@Bargs I'll have to check. Perhaps the fatal error issue was fixed.

@Bargs Bargs force-pushed the addMaxShardRequestsSetting branch from 5100c93 to dbf4f9c Compare September 10, 2018 17:20
@Bargs
Copy link
Contributor Author

Bargs commented Sep 10, 2018

Ok, I added a couple tests as well. Lemme know what you find.

@epixa
Copy link
Contributor

epixa commented Sep 10, 2018

The fatal error is still happening for me:
screen shot 2018-09-10 at 1 27 51 pm

Firefox 62/macOS

I can reproduce consistently by starting up a fresh Kibana:

  1. Enable the sample flight data linked off of the homepage.
  2. Go to advanced settings and set to courier:maxConcurrentShardRequests to -1 and save.
  3. Click on Discover
  4. --- crash ---

It also breaks on Dashboard.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs
Copy link
Contributor Author

Bargs commented Sep 10, 2018

@epixa Have you pulled my most recent changes? I rebased before my last push, so perhaps that fixed something inadvertently. I still can't get that fatal error to happen with the exact same data, OS, and browser.

nofatal

Copy link
Contributor

@epixa epixa 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 originally didn't see a merge and didn't notice your rebase, so I didn't pull down the latest. Pulled down now and I see the same thing you see, which works for me since the error message is clear and the user can navigate back to fix it.

@Bargs Bargs merged commit 15322e7 into elastic:master Sep 11, 2018
@Bargs Bargs deleted the addMaxShardRequestsSetting branch September 11, 2018 15:23
Bargs added a commit to Bargs/kibana that referenced this pull request Sep 11, 2018
…ic#22379)

Allows Kibana users to configure the max_concurrent_shard_requests param used by Kibana when sending _msearch requests. Exposes the config as an advanced setting. By default we won't send the param at all, relying on the ES default instead.
Bargs added a commit that referenced this pull request Sep 11, 2018
… (#22930)

Allows Kibana users to configure the max_concurrent_shard_requests param used by Kibana when sending _msearch requests. Exposes the config as an advanced setting. By default we won't send the param at all, relying on the ES default instead.
@s1monw
Copy link

s1monw commented Sep 11, 2018

thanks @Bargs @epixa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments