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

Allow partial results flag to be set #291

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

kahuang
Copy link

@kahuang kahuang commented Oct 30, 2018

This is useful in the case where n shards are failing to return data, but partial results are acceptable to use

@maitesin
Copy link

maitesin commented Nov 1, 2018

Hi @kahuang,

Thanks for your contribution, but can you write some test that checks the functionality you just added?

@kahuang
Copy link
Author

kahuang commented Nov 1, 2018

Happy to add tests, what kind of tests did you have in mind?

As this is just allowing us to set a flag that already existed in code, and I don't see other tests for some of the other flags (or I might just be missing it?) I'm not sure how to proceed

@maitesin
Copy link

maitesin commented Nov 6, 2018

Hi @kahuang ,

The test I had in mind was one that proofed that you can actually obtain partial results.

@eminano
Copy link

eminano commented Nov 6, 2018

Hi @kahuang,

The testing harness should allow you to setup a test with multiple shards where you can have a query with partial results enabled, so you can make sure the function you've added works as expected.

Please do target development branch instead of master.

Thanks!
Esther

@kahuang kahuang changed the base branch from master to development November 11, 2018 20:46
@kahuang
Copy link
Author

kahuang commented Nov 11, 2018

I've added a separate sharded cluster to the startdb process where I insert some test data, then shutdown one of the nodes.

The test then connects to this sharded cluster via a mongos and confirms that it gets the partial results

@eminano eminano requested review from maitesin and removed request for szank November 20, 2018 14:02
@kahuang
Copy link
Author

kahuang commented Nov 26, 2018

Any concerns on this PR?

@eminano
Copy link

eminano commented Nov 27, 2018

Hi @kahuang,

Thanks for taking the time to add the tests for this. However the build is failing due to your changes, I believe you've forgotten some setup for your sharded cluster.

Could you please have a look at how other testing clusters are done to see what's missing?

Thanks,
Esther

@kahuang
Copy link
Author

kahuang commented Nov 27, 2018

Looks like I accidentally force-pushed a commit that didn't include the daemons. I've added those now, we'll see if the tests pass now

@eminano
Copy link

eminano commented Jan 16, 2019

Hi @kahuang,

It looks like the build is still failing for mongo versions 3.0 and 3.2, any chance those issues are compatibility related?

Thanks,
Esther

@jacksontj
Copy link

Did a bit of looking, it seems that in <=3.2 mongo actually called the flag partial (it was changed here ). Likely we can send both flags-- as mongo will read the one that it knows.

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