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.

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

Abort request when too many Druid filters are generated #690

Merged
merged 3 commits into from
May 23, 2018

Conversation

QubitPi
Copy link
Contributor

@QubitPi QubitPi commented May 8, 2018

No description provided.

private static final Logger LOG = LoggerFactory.getLogger(ConjunctionDruidFilterBuilder.class);
private static final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance();

private static final int DEFAULT_MAX_NUM_DRUID_FILTERS = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is WAY too small a default. Any system that can't handle 10 values isn't
going to last 5 minutes in any production environment. I would make this
at least 10000, but probably 100,000. Basically, I think a default ceiling
should be very high. High enough that you'll start experiencing some pain before
you hit the ceiling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

High enough that you'll start experiencing some pain before
you hit the ceiling.

This can be dangerous because the pain is likely felt across multiple requests as a generally elevated latency. I think it's OK to have the default limit start very high (eg. 100k) when the feature is introduced, since that should generally be backwards-compatible, but I think the default limit should be lowered for new installations after some sane period of time.

What I'm essentially saying is that we should deprecate the high limit and lower it in a later version. That gives folks time to adjust the config value as needed, while also retaining a general policy of "sane by default".

I agree than the "safe" limit needs to be much higher than 10, but 100k seems a bit too high, since that would generate a massive query that would incur ser/deser and over-the-wire latency. 1k seems reasonable, but I could be convinced 10k is sane if there were some "here's the impact that size has on latency" data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdeszaq I have an issue created for your request - #705

private static final Logger LOG = LoggerFactory.getLogger(ConjunctionDruidFilterBuilder.class);
private static final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance();

private static final int DEFAULT_MAX_NUM_DRUID_FILTERS = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd love to see this value show up in the default config file as well, so that it's obvious it's a configurable value.

@QubitPi QubitPi force-pushed the issue-689-too-many-lucene-results branch from 9bcbee0 to 08670ac Compare May 23, 2018 01:54
@yahoo yahoo deleted a comment May 23, 2018
@QubitPi QubitPi merged commit 540a01d into master May 23, 2018
@QubitPi QubitPi deleted the issue-689-too-many-lucene-results branch May 23, 2018 14:50
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.

None yet

3 participants