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

Scalable update index #1517

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ggaughan

ggaughan commented Jun 7, 2017

Optimised update_index for partitioning large datasets (for #1514)

@wetneb

This comment has been minimized.

Show comment
Hide comment
@wetneb

wetneb Jul 22, 2017

@ggaughan Thanks a lot, I had the same problem and ended up coding my own version of update_index in my project to bypass the restrictions of the existing one.

@acdha it would be great if this could be merged!

wetneb commented Jul 22, 2017

@ggaughan Thanks a lot, I had the same problem and ended up coding my own version of update_index in my project to bypass the restrictions of the existing one.

@acdha it would be great if this could be merged!

@RabidCicada

This comment has been minimized.

Show comment
Hide comment
@RabidCicada

RabidCicada Aug 30, 2017

Contributor

@ggaughan Hey Greg...I haven't viewed the contents of this at all...but if it's finished. Could you update it to remove the conflicts due to time rot?

Then we can bug @acdha for some more attemtion:)

Contributor

RabidCicada commented Aug 30, 2017

@ggaughan Hey Greg...I haven't viewed the contents of this at all...but if it's finished. Could you update it to remove the conflicts due to time rot?

Then we can bug @acdha for some more attemtion:)

Greg Gaughan and others added some commits May 10, 2017

Process models in a consistent, alphabetical, order
Sort models by label

Ignore empty results

update_index to page through key batches without need for count (less fragile batching)

Backwards-compatible object label

Unicode fix

(try to kick of Travis)
@wetneb

This comment has been minimized.

Show comment
Hide comment
@wetneb

wetneb Sep 8, 2017

Thanks @ggaughan for updating the PR! It looks good to go now.

wetneb commented Sep 8, 2017

Thanks @ggaughan for updating the PR! It looks good to go now.

@acdha

This looks good to me. The only thing I wanted to check first was confirming that this code is exercised in a testcase which has a non-numeric PK so we can avoid a possible regression for the people using things like UUIDs.

Show outdated Hide outdated haystack/management/commands/update_index.py Outdated
Show outdated Hide outdated haystack/utils/app_loading.py Outdated
@ggaughan

This comment has been minimized.

Show comment
Hide comment
@ggaughan

ggaughan Sep 8, 2017

Not sure about the non-numeric PK test (there seems to be a MockUUIDModel that could cover it, but it may not be in the tests)

ggaughan commented Sep 8, 2017

Not sure about the non-numeric PK test (there seems to be a MockUUIDModel that could cover it, but it may not be in the tests)

@RabidCicada

This comment has been minimized.

Show comment
Hide comment
@RabidCicada

RabidCicada Sep 8, 2017

Contributor

Apologies. The mockuuidmodel is not actually used yet. I'm going to get it fixed in my next PR to address #1543 . I have had a lot of real life calamity and chaos this last week so I didn't get to finish it. Will work on it as soon as real life stuff calms down.

Contributor

RabidCicada commented Sep 8, 2017

Apologies. The mockuuidmodel is not actually used yet. I'm going to get it fixed in my next PR to address #1543 . I have had a lot of real life calamity and chaos this last week so I didn't get to finish it. Will work on it as soon as real life stuff calms down.

@RabidCicada

This comment has been minimized.

Show comment
Hide comment
@RabidCicada

RabidCicada Sep 15, 2017

Contributor

I've just created a PR #1551 to address Issue #1543 which also implements UUIDMockModel testing with a variety of tests. Hopefully this will provide coverage here. I changed the name of the uuid model to fit naming conventions with the rest of the stuff I see.

When the PR gets accepted I would ask that you quickly rebase and run the tests through travis. Or simply re-trigger a test build on this PR in travis.

Contributor

RabidCicada commented Sep 15, 2017

I've just created a PR #1551 to address Issue #1543 which also implements UUIDMockModel testing with a variety of tests. Hopefully this will provide coverage here. I changed the name of the uuid model to fit naming conventions with the rest of the stuff I see.

When the PR gets accepted I would ask that you quickly rebase and run the tests through travis. Or simply re-trigger a test build on this PR in travis.

@RabidCicada

This comment has been minimized.

Show comment
Hide comment
@RabidCicada

RabidCicada Sep 27, 2017

Contributor

alright...one more rebase and if tests pass, I think this passes @acdha 's requirements:)

Contributor

RabidCicada commented Sep 27, 2017

alright...one more rebase and if tests pass, I think this passes @acdha 's requirements:)

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