I was going to say "put cache stuff in bu.cache, and other stuff in bu.data"
2021-04-01 09140, 2021
alastairp
yeah, and that's what RedisListenStore is
2021-04-01 09120, 2021
_lucifer
there's essentially no difference between the two though. unless we would switch one to another solution
2021-04-01 09140, 2021
alastairp
yeah, which is what the chat log discussion linked from the ticket was about
2021-04-01 09147, 2021
alastairp
basically a question about naming things š±
2021-04-01 09148, 2021
_lucifer
yeah right
2021-04-01 09155, 2021
_lucifer
hah!
2021-04-01 09157, 2021
alastairp
I'm trying to step this through. We need sets in the cache component, because we've seen a case where we need to use them. This means that keys and values should be encoded here.
2021-04-01 09133, 2021
alastairp
but what I'm trying to work out is if we should have a separate interface to redis that doesn't encode values. that just exposes the raw redis commands
2021-04-01 09101, 2021
alastairp
why would this be useful? Not sure yet. maybe we'd want to explicitly work with a hash, or with a set, or with some other data structure.
2021-04-01 09125, 2021
_lucifer
well, we do provide the encode argument. just pass it as false if you don'twant the encoding
2021-04-01 09145, 2021
alastairp
oh, right. I missed that! mm
2021-04-01 09134, 2021
_lucifer
another thing is why encode keys? enforcing a key to string seems to be reasonable
2021-04-01 09146, 2021
_lucifer
*be a string
2021-04-01 09104, 2021
alastairp
again, this may be a throwback to memcache
2021-04-01 09129, 2021
alastairp
the key encoding stuff goes all the way back to 2016
2021-04-01 09145, 2021
_lucifer
yeah right, that function was never updated when we switched to redis
2021-04-01 09117, 2021
alastairp
but I do see some values in ensuring that we have ascii keys
2021-04-01 09139, 2021
alastairp
however, they could stay as strings
2021-04-01 09118, 2021
_lucifer
agreed.
2021-04-01 09127, 2021
alastairp
OK, this sounds like a bit of a direction forward. string keys. add value encoding to h* and s*, with the encode argument to disable it if desired
2021-04-01 09103, 2021
_lucifer
another thing I was thinking make the cache module a class
2021-04-01 09129, 2021
_lucifer
so that we can disable/enable encoding on a class level instead of passing it to each function
2021-04-01 09114, 2021
_lucifer
this would reduce the chance of mistakes, say we encoded while storing but forgot to decode while retrieving
2021-04-01 09148, 2021
alastairp
which is getting a bit silly, because by that point we're basically at the Redis() class again :)
2021-04-01 09157, 2021
_lucifer
yeah...
2021-04-01 09148, 2021
alastairp
let's think of this in terms of the current release that we're trying to get out.
2021-04-01 09158, 2021
alastairp
string keys + sets + encoding values is a good start
2021-04-01 09115, 2021
_lucifer
one thing i noticed is that redis encodes values to bytes while storing and does not decode them while retrieving
2021-04-01 09121, 2021
alastairp
that'll allow us to get a new BU version out soon, and update CB + LB rate limiting
2021-04-01 09132, 2021
_lucifer
yeah sounds good
2021-04-01 09115, 2021
alastairp
interesting. I see that by default it returns bytes
2021-04-01 09135, 2021
alastairp
but r = Redis(decode_responses=True) will make it convert to string
2021-04-01 09142, 2021
alastairp
of course, we can't use that if we still use msgpack
2021-04-01 09121, 2021
alastairp
_lucifer: tomorrow is a holiday here and I'll likely be taking most of the day off. what is your plan?
2021-04-01 09130, 2021
_lucifer
i'll be around
2021-04-01 09157, 2021
alastairp
is there anything you want to discuss in the next hour in order to have things to do tomorrow (and monday, which is also a holiday)
2021-04-01 09147, 2021
_lucifer
the cache works seems to have a lot of potential for improvements
2021-04-01 09103, 2021
_lucifer
let's finalize everything on the cache front
2021-04-01 09119, 2021
_lucifer
also, did you push the tests branch to unify unit tests and integration tests? let's discuss that if time permits after cache.
2021-04-01 09121, 2021
bitmap
reosarevok: I suppose just wait on MBS-11547 for now
let's make sure that we don't get too bogged down in "improving" the cache. I make this mistake often, you want to think about all of the possible situations and make it perfect, and you end up spending a huge amount of time doing nothing in the end because you didn't need it
2021-04-01 09122, 2021
alastairp
I see a value in changing the keys to strings, but other than the PR that you already opened I don't think we should do much more, it works well in the small number of cases that we use it
2021-04-01 09139, 2021
_lucifer
cool, makes sense.
2021-04-01 09117, 2021
_lucifer
i should still add the encoding parameter to it, right?
2021-04-01 09144, 2021
alastairp
yes
2021-04-01 09150, 2021
alastairp
just added a review
2021-04-01 09152, 2021
_lucifer
š
2021-04-01 09131, 2021
alastairp
for the CB review - I added "I'll make this change" because at the time I thought that the review would be quick, but in the end it was quite big
2021-04-01 09156, 2021
alastairp
so if you have time and want to update this PR to use sets and make other changes that we discussed, feel free to address all comments
2021-04-01 09157, 2021
_lucifer
I'll take of that PR. no worries.
2021-04-01 09115, 2021
_lucifer
should we do special casing now that we have sets?
I'm not sure why we specifically need sets for this
2021-04-01 09152, 2021
alastairp
isn't it just an http request with no entity id?
2021-04-01 09107, 2021
_lucifer
yes
2021-04-01 09138, 2021
_lucifer
but there are filters like sort, type etc that can still be passed
2021-04-01 09117, 2021
alastairp
what would the behaviour be here? all requests with no entity id get cached. their keys (user_id, sort, limit, offset, language) are stored in a special list key, ws_cache_blah
2021-04-01 09134, 2021
alastairp
when we expire keys for ws_cache{entity_id}, we also expire all keys in ws_cache_blah
2021-04-01 09141, 2021
_lucifer
yes
2021-04-01 09103, 2021
alastairp
just a question - do you know why userid is in the cache key?
2021-04-01 09120, 2021
_lucifer
no, it was that way before me
2021-04-01 09139, 2021
alastairp
ah, I guess it's None for an anonymous user
2021-04-01 09150, 2021
_lucifer
yes that's right
2021-04-01 09105, 2021
alastairp
I was also thinking that this seems like a _huge_ amount of screwing around for a website that's barely used, and has such a small database :)
i see we return user info as well if authenticated
2021-04-01 09121, 2021
alastairp
that user info is who wrote the review, no?
2021-04-01 09152, 2021
alastairp
JOIN "user" ON review.user_id = "user".id
2021-04-01 09154, 2021
_lucifer
ah yes right
2021-04-01 09105, 2021
alastairp
this also reminds me - we should open a ticket for this
2021-04-01 09130, 2021
alastairp
this is a possible data leak issue like we had in musicbrainz. no reason to select a user's email address when we're just showing their username on a review
2021-04-01 09142, 2021
alastairp
it'd be safer to just not return this information
2021-04-01 09150, 2021
alastairp
oh. maybe we need it to show the gravatar. that's annoying
2021-04-01 09129, 2021
alastairp
never mind.
2021-04-01 09141, 2021
alastairp
are you good for the next few days, then? I need to go
2021-04-01 09144, 2021
_lucifer
i'll need to look deeper to see if we can avoid it
2021-04-01 09103, 2021
_lucifer
yeah sure.
2021-04-01 09115, 2021
_lucifer
Happy holidays!
2021-04-01 09131, 2021
alastairp
in this case we should select it, and then convert it to a gravatar url immediately when we create the `row["user"] = User({`, and not include the email address
yvanzo: I'm updating beta - any issues with updating the translations straight on the beta branch too rather than master?
2021-04-01 09104, 2021
yvanzo
reosarevok: no, quite the contrary
2021-04-01 09116, 2021
ruaok
_lucifer: did this also include the consul changes?
2021-04-01 09137, 2021
ruaok
jasondk: hit me!
2021-04-01 09137, 2021
_lucifer
ruaok, yes.
2021-04-01 09101, 2021
ruaok
also the container startup stuff?
2021-04-01 09112, 2021
ruaok gets really optimistic now
2021-04-01 09115, 2021
_lucifer
no, not that.
2021-04-01 09133, 2021
ruaok
oh, well something to look forward to.
2021-04-01 09103, 2021
jasondk
ruaok: Hey, I'm looking into the CritiqueBrainz integration project. It looks like the CB API supports fetchingĀ reviews by a CB user uuid but not by musicbrainz username that can be associated with LB.
2021-04-01 09115, 2021
_lucifer
that PR is still open but I work with alastairp on it next week,
