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.

Sign up for GitHub

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

Jump to bottom

Add supporter metadata to user #3077

Merged
merged 6 commits into from
May 11, 2022
Merged

Add supporter metadata to user #3077

merged 6 commits into from
May 11, 2022

Conversation

rickyrombo
Copy link
Contributor

@rickyrombo rickyrombo commented May 11, 2022 •
edited
Loading

Description

  • Adds supporter_count and supporting_count to aggregate user
    • Updates aggregate_user within index_aggregate_user_tips, which I don't love...?
  • Adds does_current_user_support and does_support_current_user to populate_user_metadata
  • Makes CREATE TABLE, CREATE INDEX, ADD COLUMN, DROP TABLE, DROP INDEX, DROP COLUMN operations idempotent by default

Tests

Tested on remote dev against /tips endpoint. Also indexed an enormous amount of tips (2mil) in one go and finished successfully.

How will this change be monitored? Are there sufficient logs?

We can see how much this affects current API speed using the same record_metrics that already exists. The indexing side is also reusing existing logs to measure efficiency.

TODO:

  • Make migration idempotent

sddioulde
sddioulde approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@sddioulde sddioulde 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

rickyrombo
rickyrombo commented May 11, 2022
View reviewed changes
discovery-provider/alembic/env.py Outdated
output = re.sub("DROP (TABLE|INDEX)", r"DROP \g<1> IF EXISTS", output, re.S)
elif isinstance(element, DropColumn):
output = visit_drop_column(element, compiler, **kw)
if not hasattr(element, "element") or element.element.info.get("if_exists"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking of making add/drop columns idempotent by default, as alembic doesn't give us an element with kwargs to work with here. In which case, it might make sense to make create/drop index/table idempotent by default too? CC: @isaacsolo / @jowlee ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that makes sense to me! this is pretty cool. are you suggesting we use ORM for DDL stuff instead of raw SQL?

Copy link
Contributor Author

@rickyrombo rickyrombo May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ultimate goal is to write declarative models then use alembic revision --autogenerate and have migrations made for us!

piazzatron
piazzatron approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@piazzatron piazzatron left a comment

Choose a reason for hiding this comment

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

LGTM, ignore my other comment about the does_current_user_follow work since we're reverting those changes for now

discovery-provider/src/queries/query_helpers.py Outdated Show resolved Hide resolved
@rickyrombo rickyrombo added the discovery-node Discovery Node (previously known as Discovery Provider) label May 11, 2022
@rickyrombo rickyrombo merged commit b32512f into master May 11, 2022
@rickyrombo rickyrombo deleted the mjp-aggregate-user-support branch May 11, 2022 22:27
sliptype pushed a commit that referenced this pull request Sep 10, 2023
@rickyrombo
Clean up NPM package lock (#3077)
82a3df6
sliptype pushed a commit that referenced this pull request Sep 10, 2023
v1.5.17
6189ab3 
[e804aa1] [C-2248, C-2373] Use playlistUpdates, remove legacyNotifications (#3094) Dylan Jeffers
[824933e] [C-2366] Improve web notification selection performance (#3103) Dylan Jeffers
[4b8edef] [PLAT-696] Add trending-playlists/underground notifications (#3089) Dylan Jeffers
[1f9cf3e] [C-2275] Fix android drawer offsets (#3095) Dylan Jeffers
[fc14c82] [PAY-1063][PAY-1085][PAY-1086] Update UI for inaccessible gated tracks from favorites and history pages (#3100) Saliou Diallo
[b0441f5] [C-2365] Update play buttons on web and mobile to show resume when track is current (#3101) Kyle Shanks
[453910f] [C-2378] Add upload v2 feature flag (#3099) Sebastian Klingler
[962a6df] [C-2337] Remove reachability mobile web (#3090) Raymond Jacobson
[4ad5cd2] Fix visible collectibles for upload popup (#3093) Saliou Diallo
[c143078] Fix feature flag bug (#3092) Saliou Diallo
[44435b5] Fix upload prompt modal learn more url (#3091) Saliou Diallo
[c9024ad] Use chat.messagesStatus instead of selector (#3087) Reed
[38d43c4] [C-2369] Fix issue where notification poll can break app on signout (#3088) Dylan Jeffers
[90122d9] [PAY-923] DMs: Add desktop entrypoints (#3083) Marcus Pasell
[00f27e8] [PAY-907] Mobile chat reactions (#3020) Reed
[4678b89] DMs: Fix broken typecheck on main (#3086) Marcus Pasell
[756ade4] [PAY-1000][PAY-1084][PAY-1096][PAY-1097][PAY-1098] - More gated content fixes (#3085) Saliou Diallo
[820aa9d] Fix upload and repost probers tests and lint (#3076) Sebastian Klingler
[345607e] [C-2320] Fix profile socials alignment (#3079) Dylan Jeffers
[569199c] Fix prod build timeout (#3084) Sebastian Klingler
[12f6c22] Remove ports for local dev (#3082) Theo Ilie
[1940618] Fix broken Main build due to typeerror (#3080) Marcus Pasell
[eb8d47e] [PAY-1082] DMs: Dedupe sent messages (#3066) Marcus Pasell
[50a11c3] Update SDK to 2.0.3-beta.0 (#3078) Marcus Pasell
[c420fbb] Clean up NPM package lock (#3077) Marcus Pasell
[35d1124] [C-2327] Add playlist updates slice (#3063) Dylan Jeffers
[59862ad] [C-2344] Update the web playbar scrubber to respect the playback speed of podcasts (#3075) Kyle Shanks
[ffeb0d3] [C-2349] Default download on wifi only to false (#3074) Andrew Mendelsohn
[cafae41] [C-2325] Fix playlist table date-added column (#3073) Dylan Jeffers
[384a510] [PAY-927] DMs: Empty messages state (#3068) Marcus Pasell
[1132f83] Update @jup-ag/core to 2.0.0-beta.9 (#3072) Marcus Pasell
[49c0ebf] [PAY-1072] Change "Download App" icon on Settings Page (#3067) Marcus Pasell
[928dcaf] [PAY-1056] - More gated content updates and fixes (#3070) Saliou Diallo
[1e1f769] [C-2345] Move PlaybackRate drawer to common drawers map (#3071) Kyle Shanks
[f5d1251] Fix web-dist CI steps (#3069) Sebastian Klingler
[5f89800] Fix heavy rotation playlist on client (#3056) sabrina-kiam
[c0191e2] [C-2316] Add remote config for all oauth verification (#3052) Raymond Jacobson
[40f5627] [PAY-1074][PAY-1075][PAY-1076][PAY-1080] - Update availability settings states + more QA fixes (#3059) Saliou Diallo
[5be60ac] [C-2339] Update podcast control updates to also work for audiobooks (#3065) Kyle Shanks
[163ebf5] [C-2297] Add fallback flag to podcast feature (#3064) Sebastian Klingler
[f206391] [PAY-904] - Add gated content upload prompt (#3057) Saliou Diallo
[1afc4e5] [C-1344] Move probers to monorepo and make tests pass (#3061) Sebastian Klingler
[e198279] Remove random line (#3062) Saliou Diallo
[24a001b] Add playback position logic for mobile (#3051) Kyle Shanks
[d210124] [PAY-1070] Update TabSlider/SegmentedControl slider size on resize (#3044) Marcus Pasell
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaacsolo isaacsolo isaacsolo left review comments

@piazzatron piazzatron piazzatron approved these changes

@sddioulde sddioulde sddioulde approved these changes

Assignees

@dylanjeffers dylanjeffers

@sddioulde sddioulde

Labels
discovery-node Discovery Node (previously known as Discovery Provider) size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rickyrombo @isaacsolo @piazzatron @sddioulde @dylanjeffers