Hey ruaok , if you are free, would you review my PR?
ruaok
will do today, riksucks!
riksucks
thanks ruaok :D
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.
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. :)
riksucks: done -- I have some questions
yvanzo: do you know if MB editor search is exposed via the MB WS?
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.
*that WS endpoint if there was one
ruaok
good point. It just that LIKE queries don't scale well (table scan) and that we already have editor search implemented.
maybe if we add a text search index to the LB DB?
lucifer
never used a text search index before so not sure. but no harm in trying it :D
ruaok
I would approve of that approach, but ILIKE will blow up in our faces in the future.
lucifer
let's do that for now as that sounds simple to do. we can deal with scale issues later?
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.
ruaok
the problem with scale issues is that they bite you in the ass when you're busy doing other things.
I've done the "we'll worry about the scale issues when they come" before and it ends up being a time-bomb.
either we do no search at all, or we do it right. I dont want to build in time-bombs.
lucifer
yeah :/. to be sure, you mean ILIKE and the text search index both have scalability issues or only the former?
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.
a text index will scale much better.
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".
lucifer
oh. big corps are weird.
ruaok
poor management rules will yield poor products.
lucifer
+1
anyone has an example of a MB username with non ASCII characters?
ruaok
reosarevok: ^^ ?
Etua has quit
lucifer
nvm, i used a fake one.
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?
that's great!
lucifer
yup create extension `pg_trgm`;
reosarevok
Sorry, I was away. I'm sure I could find some but I guess there's no need anymore? :)
ruaok
reosarevok: nope. thanks.
lucifer: sounds like a good solution then, lets go for it.
riksucks
ruaok lucifer
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
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
that should be it, right?
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?
ruaok
seems fine, yes, lucifer
lucifer
👍
ruaok
riksucks: yep, sounds good.
monkey
Happy to see some trial of PG-based fuzzy search at scale, I've been curious about that
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.
opal has quit
opal joined the channel
Etua joined the channel
riksucks
ruaok lucifer
I have committed according to the review. Do drop review whenever you guys are free.
yvanzo
reosarevok: Is #2198 ready for review? or work in progress? or waiting for other PR?
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?
bitmap suggested ways to do that but they require thinking, so probably not happening today :D
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.
reosarevok
yvanzo: wanna leave a comment with the ones you've noticed should be replaced?
I'll look into it once I'm done with these URL PRs
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.
ruaok: did you get a chance to look at the charts on LB website?
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
(unless I'm missing a good way to make the other vimeo string more specific)
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?
I guess I'll send it like that for now, note it on the commit msg
dseomn has quit
dseomn joined the channel
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-serve... 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
yvanzo
reosarevok: Not a better way but what is commonly used in this file: switch/case. The URL format should be checked as well.
Yes, let's do the milestone after the freeze only, even tomorrow if needed.