bitmap, yvanzo ^ not too big a deal, but a regression from this release nevertheless
2021-04-21 11135, 2021
reosarevok has quit
2021-04-21 11137, 2021
reosarevok_ joined the channel
2021-04-21 11151, 2021
revi has quit
2021-04-21 11107, 2021
Freso has quit
2021-04-21 11119, 2021
revi joined the channel
2021-04-21 11120, 2021
wilbur joined the channel
2021-04-21 11130, 2021
Freso joined the channel
2021-04-21 11148, 2021
_lucifer
alastairp: ruaok: in addition to the cache discussion, it would be nice to discuss datetime serialization as well today.
2021-04-21 11110, 2021
_lucifer
afaik we cannot store timestamp fields directly in jsonb, but for the youtube player we need that functionality in some places. so i was thinking to convert the date to isoformat and store that.
2021-04-21 11148, 2021
_lucifer
but would be nice to solve this in one place (maybe BU?) and use that everywhere else
2021-04-21 11126, 2021
sumedh has quit
2021-04-21 11112, 2021
alastairp
_lucifer: mmm, interesting idea.
2021-04-21 11158, 2021
alastairp
so the field that you wan't isn't generic enough to go in a regular column?
2021-04-21 11150, 2021
alastairp
is this something that the youtube api needs, or something that you need to keep track of to do an operation? does the youtube api take it as a timestamp or datetime formatted field>?
2021-04-21 11112, 2021
_lucifer
its the latest_listened_at field currently only used by spotify, youtube doesn't allow to records listens so it isn't needed.
2021-04-21 11147, 2021
_lucifer
for other music services, as well we don't know if it'll be needed.
2021-04-21 11125, 2021
_lucifer
yeah, the spotify reader uses it to track the last fetched listen i think
2021-04-21 11135, 2021
alastairp
mmm
2021-04-21 11148, 2021
alastairp
I don't think that this should be in the oauth table
2021-04-21 11101, 2021
alastairp
it's related to "import listens from external services", that should be another table
2021-04-21 11153, 2021
_lucifer
ah so two tables? one only for services that allow to import listens?
2021-04-21 11109, 2021
yvanzo
mo’’in’
2021-04-21 11118, 2021
alastairp
yeah, that's what I'd do
2021-04-21 11148, 2021
reosarevok_
moin!
2021-04-21 11105, 2021
alastairp
have (user_id, service_name, service_registration_id, last_imported, error_message) or something
2021-04-21 11140, 2021
_lucifer
interesting we don't need the jsonb field in oauth table.
2021-04-21 11147, 2021
alastairp
oh great
2021-04-21 11147, 2021
_lucifer
if we make two tables
2021-04-21 11114, 2021
alastairp
I thought you were storing additional fields in here that the oauth services returned
2021-04-21 11100, 2021
_lucifer
for now its all the excess data from the existing spotify table, i don't think we'll need to store anything else there
2021-04-21 11137, 2021
_lucifer
there's a permissions column but both youtube and spotify use it and other services will probably will as well so that can become a regular column
2021-04-21 11145, 2021
_lucifer
*permissions field
2021-04-21 11150, 2021
alastairp
right
2021-04-21 11156, 2021
_lucifer
cool sounds good to me. i'll rework the tables.
2021-04-21 11129, 2021
yvanzo
chaban: I checked, it doesn’t even match your editing periods. It is more likely caused either by other services load or by a few edits that trigger a lot of reindex messages (e.g. editing crowded areas or relationship types).
2021-04-21 11103, 2021
yvanzo
reosarevok_: most probably pg load caused this, it doesn't seem to be specifically related to adding a label.
2021-04-21 11112, 2021
yvanzo
reosarevok_, bitmap: reviewed, would be nice to update beta tonight :)
alastairp: is there any benefit in storing user_id and service again here instead of a FK to external_service_oauth
2021-04-21 11141, 2021
reosarevok_ is now known as reosarevok
2021-04-21 11148, 2021
ZaphodBeeblebrox is now known as CatQuest
2021-04-21 11139, 2021
alastairp
_lucifer: I think a user id is useful, and there should be a link to external_service_auth
2021-04-21 11116, 2021
alastairp
_lucifer: depends on what we want to do with the auth row if there is a failure - our discussion about deleting spotify rows but then not being able to see an error message
2021-04-21 11152, 2021
alastairp
so, if we want to delete the external_service_oauth row, we should have `service`, so that we can still report to the user if they had an error with spotify, etc
2021-04-21 11101, 2021
alastairp
but if you want to keep an invalid auth row, perhaps service isn't needed
2021-04-21 11139, 2021
sumedh joined the channel
2021-04-21 11136, 2021
_lucifer
alastairp: makes sense, i don't see any value in keeping around invalid auth rows. so i would be fine with whatever's easier to implement, adding an invalid or deleting the row altogether.
2021-04-21 11111, 2021
alastairp
_lucifer: right, so in that case external_service_oauth_id would have to be nullable
2021-04-21 11129, 2021
_lucifer
yes, that's what i am thinking currently
2021-04-21 11134, 2021
alastairp
if a row exists in listen_importer with that value null, it means that there was an error (error should be set)
2021-04-21 11145, 2021
alastairp
and then we can use user_id, service to link that to a user
2021-04-21 11150, 2021
alastairp
yeah, this feels much better!
2021-04-21 11116, 2021
_lucifer
yup, just saw postgres has the `ON DELETE SET NULL` so its simple to implement as well
2021-04-21 11118, 2021
alastairp
yeah, exactly
2021-04-21 11140, 2021
_lucifer
also, i saw you pushed the helper file. should i add the remaining .finish files?
this is instead of the previous one? a bit more complex because it has 2 tables?
2021-04-21 11115, 2021
_lucifer
yes
2021-04-21 11125, 2021
alastairp
_lucifer: that's a beautiful query
2021-04-21 11122, 2021
_lucifer
yup, postgresql is indeed powerful :-DD
2021-04-21 11135, 2021
alastairp
CTEs are SQL standard, though
2021-04-21 11141, 2021
ruaok
indeed very nice query. I really dig crafting more complex PG queries.
2021-04-21 11144, 2021
alastairp
(but not suppored in all sql servers)
2021-04-21 11152, 2021
alastairp
*cough* mysql
2021-04-21 11113, 2021
ruaok
yeah, that looks fine, _lucifer
2021-04-21 11117, 2021
alastairp
_lucifer: I see that you've still got a bunch of instances of the json column still in that PR, so I stopped reviewing it
2021-04-21 11159, 2021
_lucifer
alastairp, yes, just was going to add that comment. the python side is still not completely migrated. i wanted to get a review on the sql first before working on that.
2021-04-21 11110, 2021
alastairp
ah cool!
2021-04-21 11153, 2021
_lucifer
alastairp: i think it makes sense to add `last_updated` field as well to listens_importer table. what do you think?
2021-04-21 11113, 2021
_lucifer
both external_service_oauth and listens_importer will have a last updated field, for the first table it will denote last auth details update and for the second the last time we tried fetching that user's listens
2021-04-21 11104, 2021
sumedh joined the channel
2021-04-21 11116, 2021
alastairp
_lucifer: yes, good idea because that is not always going to be the same as last_listened_at
2021-04-21 11106, 2021
_lucifer
alastairp: 👍, further i think we can get rid of `record_listens` field as well, because we'll be inserting only users for which we can record listens in the listens_importer table.
2021-04-21 11130, 2021
alastairp
see how much simpler things get when you add another table!
2021-04-21 11133, 2021
_lucifer
yup! makes sense a lot sense as there is a clear distinction between roles of the two tables.
2021-04-21 11155, 2021
ruaok
alastairp: waiting for tests to run. what else should I look at next?
if you happen to be able to spot the problem, plz let me know.
2021-04-21 11140, 2021
ruaok dislikes mocks
2021-04-21 11149, 2021
alastairp
urg, yeah, that sounds like bugs with the mock
2021-04-21 11150, 2021
alastairp
though looking at that test, my first gut feel is that it's wrong. we shouldn't be testing that logs are made, we should be testing some methods that return True or False
if you made this function return a tuple `max_rating > 1.0, min_rating < -1.0` then you could just assert the return values
2021-04-21 11136, 2021
ruaok
sounds sane, thanks.
2021-04-21 11101, 2021
alastairp
_lucifer: do you know how to evict things from the gh actions cache?
2021-04-21 11150, 2021
alastairp
#1398 managed to build the full listenbrainz python image and cache it for js tests 🤦♂️
2021-04-21 11108, 2021
alastairp
so now js tests restore those images on every test... takes another 20 seconds ;)
2021-04-21 11117, 2021
_lucifer
alastairp: i don't think it is currently possible to do that.
2021-04-21 11130, 2021
_lucifer
ah for that, we can change the cache key
2021-04-21 11100, 2021
alastairp
I don't think it's super important. hopefully it'll get evicted in a few days/weeks
2021-04-21 11123, 2021
_lucifer
iirc cache is reset weekly.
2021-04-21 11145, 2021
_lucifer
ah no! its like this
2021-04-21 11147, 2021
_lucifer
>A repository can have up to 5GB of caches. Once the 5GB limit is reached, older caches will be evicted based on when the cache was last accessed. Caches that are not accessed within the last week will also be evicted.
2021-04-21 11144, 2021
alastairp
but I guess the cache will be accessed every time we run a CI job
