Skip to content

Conversation

pathikdevani
Copy link

@pathikdevani pathikdevani commented Aug 5, 2016

when source to set false, Nest consider exclude["*"]. it's effect on performance.

i solve this using Union<>.

@gmarz gmarz mentioned this pull request Aug 6, 2016
@russcam
Copy link
Contributor

russcam commented Aug 7, 2016

Hey @pathikdevani, we maintain backwards binary compatibility within major versions of NEST and unfortunately this solution would break backwards binary compatibility (bwc) for 2.x; We would need to come up with another way of handling this for 2.x that maintains bwc.

This fix would be great to get into master though that is tracking against Elasticsearch 5.x; would you be able to close this PR and retarget the change against master?

@pathikdevani
Copy link
Author

@russcam thanks for replay.

It is very serious issue when we use aggregation. If we can add change in 2.x then it's very useful.
Anything i can do for add change in 2.x?

And I create new PR for master branch (5.x).

gmarz added a commit that referenced this pull request Aug 23, 2016
SourceFilter now has a Disable property which can be used to enable or
disable _source.  Instances that accepted Source(bool) now set this
property rather than ExcludeAll (which doesn't truly disable _source).

This is more of a workaround to maintain BWC in 2.x  In master, we will
use Union<bool, ISourceFilter> instead as proposed in #2200.
@gmarz
Copy link
Contributor

gmarz commented Aug 23, 2016

@pathikdevani I just opened the above PR (#2223) which proposes a fix that's backwards compatible with 2.x, which will likely make it into the next release.

I'll work on incorporating your PR into master. Thanks for raising this!

@gmarz gmarz changed the base branch from 2.x to master August 24, 2016 18:53
@gmarz gmarz changed the base branch from master to 2.x August 24, 2016 18:53
@gmarz gmarz closed this in 532aaa6 Aug 25, 2016
@gmarz
Copy link
Contributor

gmarz commented Aug 25, 2016

@pathikdevani this is now pushed to master with the above commit. Thanks again for the PR!

@pathikdevani
Copy link
Author

Thank you @gmarz

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.

3 participants