-
Notifications
You must be signed in to change notification settings - Fork 106
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
New GetTracks Routing via Handle and Slug #1655
Conversation
discovery-provider/alembic/versions/534987cb0355_recreate_track_routes_table.py
Show resolved
Hide resolved
discovery-provider/alembic/versions/c8d2be7dcccc_repair_poorly_sorted_tracks.py
Show resolved
Hide resolved
session.add(new_track_route) | ||
|
||
# Commit this so it gets in before the new route creation | ||
session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering if there could be any unintended consequences of committing here if we haven't committed other changes earlier. i think fine b/c this is at the start of the indexing flow. maybe worth a comment inline where it's called to the future editor knows that it does commit here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in the "new-style" routes method I also commit. I added some comments where these get called.
also cc: @hareeshnagaraj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rickyrombo @raymondjacobson is the reason we need to commit because inside a postgres tx we can't read data uncommitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to make sure i understand. since we now call session.commit here, one set of db records is written with user + some track changes. then the second commit writes reset of the indexing changes like ursm, socials etc.
this changes the way we do block level commits atomically for all records. does this affect rollbacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry just seeing this now. It does disrupt the unit of work model of each block. We could keep track of the unwritten routes separately in memory for each transaction, which could avoid this commit. I can add that in a PR next week?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested rollbacks - any ideas/suggestions there? - but I imagine this would disrupt that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this actually breaks anything because these commits all fire before we touch the track record, etc. The in-memory solution seems fine, but I also don't think it's necessary.
If for some reason we hit an exception after writing to the track routes table, we would effectively have considered indexing that block a failure and would retry it. Even if the routes table changes were not reverted, the next time the indexer tries to re-index that block, it would try to re update the routes and because they're (IIUC) idempotent, we would continue to make progress.
Worst-case, we have halted indexing entirely and we would skip over the block (after consensus) per @jowlee 's work and all discovery nodes would be consistent. Because the routes table model supports backlinks in various capacity, that would be fine.
This definitely all changes the "unit of work" model, but I don't think for the worse
model_dict = {} | ||
if exclude_keys is None: | ||
exclude_keys = [] | ||
def model_to_dictionary(model, exclude_keys=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should unit test this guy probs. I think it would be not that hard?
…date track indexing test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Description
What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?
Track
model (migrations should not depend on models as the model schema may change. If a migration needs to use a model, it should make it's own copy so that the dep is deterministic)track_routes
for existing tracks so that current links don't breakhandle
+slug
slug
toTrack
model using relationships to always pull the currentslug
, and updating themodel_to_dict
method to include relationships and propertiesresolve
endpoint to use new track routing conventiontrack_routes
and new tests to ensureslug
gets populated and that_get_tracks
works for getting tracks byhandle
+slug
Tests
List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗
...
❗ Reminder 💡❗:
If this PR touches a critical flow (such as Indexing, Uploads, Gateway or the Filesystem), make sure to add the
requires-special-attention
label. Add relevant labels as necessary.How will this change be monitored?
For features that are critical or could fail silently please describe the monitoring/alerting being added.