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

Generate correct number of codes in BatchBuilder #2578

Merged
merged 2 commits into from
Feb 16, 2018

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Feb 15, 2018

Previously, due to a bug in the inner loop, BatchBuilder could generate more codes than were requested.

This removes the inner loop in favour of a single, smarter outer loop. Instead of always creating exactly batch_size new codes, we now create up to batch_size new codes. On each loop we try to generate batch_size new potential codes and only save the ones which are unique.

This should result in fewer selects in scenarios where there is contention over codes. Though this is entire system is not well suited for there to be many conflicts.

Fixes #2577

Previously this was building codes on the association and then relying
on the autosave behaviour to save the new records when save was called
on the Promotion.

It's simpler to just explicitly create them.
@jhawthorn jhawthorn changed the title Generate current number of codes in BatchBuilder Generate correct number of codes in BatchBuilder Feb 15, 2018
Previously, due to a bug in the inner loop, BatchBuilder could generate
more codes than were requested.

This removes the inner loop in favour of a single, smarter outer loop.
Instead of always creating exactly batch_size new codes, we now create
up to batch_size new codes. On each loop we try to generate batch_size
new potential codes and only save the ones which are unique.

This should result in fewer selects in scenarios where there is
contention over codes. Though this is entire system is not well suited
for there to be many conflicts.
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

@jhawthorn jhawthorn merged commit 5bebfdf into solidusio:master Feb 16, 2018
@jhawthorn jhawthorn added this to the 2.5.0 milestone Feb 19, 2018
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