Hey ruaok , if you are free, would you review my PR?
2021-09-27 27026, 2021
ruaok
will do today, riksucks!
2021-09-27 27046, 2021
riksucks
thanks ruaok :D
2021-09-27 27004, 2021
lucifer
ruaok: hi! re spark dumps, the order of the fields has changed, so we should probably rearrange the fields to match older version or increment the version number in SCHEMA file.
2021-09-27 27001, 2021
ruaok
hi. Hmm, ok, but where are you expecting the artist_mbids column to be inserted? can you paste me the order you're expecting?
nice, I like dictionary value stability in python. :)
2021-09-27 27040, 2021
ruaok
riksucks: done -- I have some questions
2021-09-27 27026, 2021
ruaok
yvanzo: do you know if MB editor search is exposed via the MB WS?
2021-09-27 27008, 2021
lucifer
i tried that yesterday and i think it is not. i was thinking that LB search box could call that WS endpoint and then display results but figured there's another issue that all editors don't have a LB account so probably not the best way to implement it anyway.
2021-09-27 27023, 2021
lucifer
*that WS endpoint if there was one
2021-09-27 27010, 2021
ruaok
good point. It just that LIKE queries don't scale well (table scan) and that we already have editor search implemented.
2021-09-27 27028, 2021
ruaok
maybe if we add a text search index to the LB DB?
2021-09-27 27008, 2021
lucifer
never used a text search index before so not sure. but no harm in trying it :D
2021-09-27 27051, 2021
ruaok
I would approve of that approach, but ILIKE will blow up in our faces in the future.
2021-09-27 27021, 2021
lucifer
let's do that for now as that sounds simple to do. we can deal with scale issues later?
2021-09-27 27022, 2021
lucifer
maybe when MeB has all users, we can expose an endpoint to search users with an option for Oauth apps and limit the result to those who have approved access to them.
2021-09-27 27020, 2021
ruaok
the problem with scale issues is that they bite you in the ass when you're busy doing other things.
2021-09-27 27046, 2021
ruaok
I've done the "we'll worry about the scale issues when they come" before and it ends up being a time-bomb.
2021-09-27 27001, 2021
ruaok
either we do no search at all, or we do it right. I dont want to build in time-bombs.
2021-09-27 27003, 2021
lucifer
yeah :/. to be sure, you mean ILIKE and the text search index both have scalability issues or only the former?
2021-09-27 27012, 2021
ruaok
if you go with exact search (read no ILIKE) then you can build an index and you're done. if you want fuzzy searching you're always doing a table scan, which isn't a problem now, but will be later.
2021-09-27 27022, 2021
ruaok
a text index will scale much better.
2021-09-27 27033, 2021
lucifer
cool, let's do the text index then? an exact search is not very useful.
yeah, I read something similar and it suggested the same -- it suggested that that is how we got the touchbar on MacBooks and many other "improvements".
2021-09-27 27028, 2021
lucifer
oh. big corps are weird.
2021-09-27 27001, 2021
ruaok
poor management rules will yield poor products.
2021-09-27 27014, 2021
lucifer
+1
2021-09-27 27014, 2021
lucifer
anyone has an example of a MB username with non ASCII characters?
2021-09-27 27036, 2021
ruaok
reosarevok: ^^ ?
2021-09-27 27055, 2021
Etua has quit
2021-09-27 27031, 2021
lucifer
nvm, i used a fake one.
2021-09-27 27008, 2021
lucifer
ruaok: so i did some look up on full text search, it matches full word for user names that's not ideal. for partial search it supports only prefix matching, luc:* to match lucifer but no way to get an inword partial match or suffix match. alternatively, we can use `pg_trgm` extension to support better matches.
and installing the extension is just one command, among the others we already have?
2021-09-27 27055, 2021
ruaok
that's great!
2021-09-27 27009, 2021
lucifer
yup create extension `pg_trgm`;
2021-09-27 27009, 2021
reosarevok
Sorry, I was away. I'm sure I could find some but I guess there's no need anymore? :)
2021-09-27 27018, 2021
ruaok
reosarevok: nope. thanks.
2021-09-27 27032, 2021
ruaok
lucifer: sounds like a good solution then, lets go for it.
2021-09-27 27051, 2021
riksucks
ruaok lucifer
2021-09-27 27051, 2021
riksucks
I took a look at your discussions. So based on that I think I should remove the pin part from the docstring, and a comment
2021-09-27 27051, 2021
riksucks
also, ruaok, does `delete_user_timeline_event` seem like an okay name? Since `create_user_timeline_event` can only create notifications/recommendations, and the db method I wrote is technically the counter method of this method
2021-09-27 27051, 2021
riksucks
that should be it, right?
2021-09-27 27038, 2021
lucifer
ruaok: one catch: the index for pg_trgm is often larger than the table itself but this is a user table (small) so i guess we are covered?
2021-09-27 27021, 2021
ruaok
seems fine, yes, lucifer
2021-09-27 27025, 2021
lucifer
👍
2021-09-27 27055, 2021
ruaok
riksucks: yep, sounds good.
2021-09-27 27024, 2021
monkey
Happy to see some trial of PG-based fuzzy search at scale, I've been curious about that
2021-09-27 27023, 2021
lucifer
if it works well enough, i'd vote to push it into a future BU PG module so that all *Brainz can use it.
2021-09-27 27025, 2021
opal has quit
2021-09-27 27054, 2021
opal joined the channel
2021-09-27 27008, 2021
Etua joined the channel
2021-09-27 27002, 2021
riksucks
ruaok lucifer
2021-09-27 27002, 2021
riksucks
I have committed according to the review. Do drop review whenever you guys are free.
2021-09-27 27020, 2021
yvanzo
reosarevok: Is #2198 ready for review? or work in progress? or waiting for other PR?
2021-09-27 27009, 2021
reosarevok
Well, it's mostly ready but I should probably make sure with bitmap on what small changes he wanted
yvanzo: what do you think, should I strip the favicon changes for now from the other URL tickets so we can release them to beta while I work on refactoring the favicon bits?
2021-09-27 27018, 2021
reosarevok
bitmap suggested ways to do that but they require thinking, so probably not happening today :D
2021-09-27 27010, 2021
yvanzo
reosarevok: yes, if not happening today, set it as draft and rebase your other dependent PRs on master instead.
Most of the current descriptions are useless, but some labels are misleading and should probably be replaced by the corresponding descriptions instead.
2021-09-27 27014, 2021
reosarevok
yvanzo: wanna leave a comment with the ones you've noticed should be replaced?
2021-09-27 27027, 2021
reosarevok
I'll look into it once I'm done with these URL PRs
2021-09-27 27036, 2021
lucifer
tandy[m]: no, we just don't generate them currently. that's changing soon. in a couple of weeks i hope, it should start serving up to date stats.
2021-09-27 27033, 2021
lucifer
ruaok: did you get a chance to look at the charts on LB website?
2021-09-27 27012, 2021
reosarevok
yvanzo: heh. So, for the vimeoondemand PR, if we use the current favicon code, it will work, but AFAICT only if I place them like this in constants.js:
Since it will catch the first if it matches, and not look any further
2021-09-27 27058, 2021
reosarevok
(unless I'm missing a good way to make the other vimeo string more specific)
2021-09-27 27029, 2021
reosarevok
Seems fine to me since it won't last long until we replace it with something better, hopefully, and still unbreaks adding those links, but just making sure you feel that's ok?
2021-09-27 27029, 2021
reosarevok
I guess I'll send it like that for now, note it on the commit msg
2021-09-27 27022, 2021
dseomn has quit
2021-09-27 27021, 2021
dseomn joined the channel
2021-09-27 27002, 2021
yvanzo
reosarevok: yes, the vimeo PR is still missing some checks in the function "validate"
Also, I put a couple things on https://github.com/metabrainz/musicbrainz-server/… but I guess we should see what we can merge from the current one before we freeze and move the rest before we decide 100% on what to add
2021-09-27 27050, 2021
yvanzo
reosarevok: Not a better way but what is commonly used in this file: switch/case. The URL format should be checked as well.
2021-09-27 27051, 2021
yvanzo
Yes, let's do the milestone after the freeze only, even tomorrow if needed.
