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

Ability to skip Incremental Index during query using query context #1957

Merged
merged 1 commit into from
Nov 24, 2015

Conversation

nishantmonu51
Copy link
Member

  • add adds the ability to skip incremental index when querying results from realtime nodes.
  • default behaviour is to include incrementalIndex in queries.

@fjy
Copy link
Contributor

fjy commented Nov 12, 2015

can you explain the logic behind this PR?

@drcrallen
Copy link
Contributor

I'm guessing its because this context setting causes the query to only hit persisted segments, which should be faster. If you don't care about absolute-up-to-the-second data you can skip hitting the incremental index which might be heavily hit due to ingestion.

@nishantmonu51 can you get some performance metrics around this?

@nishantmonu51
Copy link
Member Author

@fjy, @drcrallen, yeah its correct, the idea behind this is to help with query performance when you don't want data upto the latest second and can tolerate delay upto the intermediatePersistPeriod,
the general idea on how it will help is -

@nishantmonu51
Copy link
Member Author

@drcrallen will look into getting some performance numbers.

@xvrl
Copy link
Member

xvrl commented Nov 12, 2015

@fjy tldr; it's the difference between real-time and real²time :)

@@ -291,6 +294,7 @@ private Sink getSink(long timestamp)

// The realtime plumber always uses SingleElementPartitionChunk
final Sink theSink = holder.getObject().getChunk(0).getObject();
final boolean skipIncrementalSegment = query.getContextValue(SKIP_INCREMENTAL_SEGMENT, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will fail with class cast exception if the value is a string instead of a boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, it might be useful to get it once on top and reuse instead of getting for each sink.

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 think we only support Strings for backwards compatibility, do we have to support strings for new properties ?

This PR adds adds the ability to skip incremental index when querying
results from realtime nodes. default behaviour is to include
incrementalIndex in queries.

review comment
@himanshug
Copy link
Contributor

can you document the new attribute?

@fjy
Copy link
Contributor

fjy commented Nov 18, 2015

@nishantmonu51 @xvrl I don't really understand the purpose of this PR? Is it so you guys can avoid querying the in memory component of Druid's realtime? Why not look into why the in-memory component is slow?

@gianm
Copy link
Contributor

gianm commented Nov 20, 2015

👍

@xvrl
Copy link
Member

xvrl commented Nov 24, 2015

👍 it's helpful to have to investigate performance problems. Probably should not need to be documented, since it's behavior is dependent on real-time ingestion implementation details. If it proves very useful, we can promote it.

@xvrl xvrl removed the Discuss label Nov 24, 2015
gianm added a commit that referenced this pull request Nov 24, 2015
Ability to skip Incremental Index during query using query context
@gianm gianm merged commit 13af260 into apache:master Nov 24, 2015
@nishantmonu51 nishantmonu51 mentioned this pull request Dec 1, 2015
@gianm gianm added this to the 0.8.3 milestone Dec 1, 2015
@gianm gianm mentioned this pull request Dec 4, 2015
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants