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

Rebase mqtt 0.15 #3

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

jasinner
Copy link

Rebasing on upstream django-mqtt 0.15 (actually the main branch) but 0.15 was the last tag.

@jasinner
Copy link
Author

jasinner commented Oct 16, 2023

@juspence please review. There's no particular reason to rebase. I was jus want to make sure we're staying up to date with the most recent changes in django-mptt upstream.

@juspence
Copy link
Collaborator

juspence commented Oct 17, 2023

@juspence please review. There's no particular reason to rebase. I was jus want to make sure we're staying up to date with the most recent changes in django-mptt upstream.

I really wish we could do this...there was even a PR merged just after I created this fork originally, but I couldn't rebase it due to lots of conflicts. I think those conflicts are still present and need to be resolved first.

The bigger problem is I'm not sure what the upstream changes are. We're using a very hacked-together commit that uses UUIDs instead of integers as node IDs (or maybe tree IDs, or maybe both, don't remember which). This avoids an autoincrementing primary key when creating nodes, so node creation can happen concurrently / in parallel across different Celery tasks / workers.

Without this commit, we either have to create ProductNodes and ComponentNodes one at a time (which will be very slow), or else trees may get corrupted and need to be rebuilt when two nodes happen to be created at the same time (so share the same ID and end up participating in the wrong trees).

Upstream didn't want to accept this change, so we're forced to keep this commit separate. If we rebase lots of changes, I'd be worried that we'd need to make this ID change (look for a UUID instead of an int) in other / new places, so that the existing code for this change continues to work correctly. If we miss one of those spots, or if we resolve a conflict incorrectly, our existing code will probably break and start creating corrupt trees, but we likely won't notice right away.

I think our effort is best spent on abandoning use of Django-MPTT - e.g. when loading prod-defs data, we create products / versions / streams / variants that are directly linked to each other using ForeignKeys. So we may not need a tree to represent the product taxonomy. Then all queries to the ProductNode model could be rewritten to use the ForeignKeys instead, and the ProductNode model could just go away.

Getting rid of the ComponentNode model is more complicated, since the tree structure is less fixed and we may add components to different places at different times.

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.

7 participants