I was going to say "put cache stuff in bu.cache, and other stuff in bu.data"
yeah, and that's what RedisListenStore is
_lucifer
there's essentially no difference between the two though. unless we would switch one to another solution
alastairp
yeah, which is what the chat log discussion linked from the ticket was about
basically a question about naming things 😱
_lucifer
yeah right
hah!
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.
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
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.
_lucifer
well, we do provide the encode argument. just pass it as false if you don'twant the encoding
alastairp
oh, right. I missed that! mm
_lucifer
another thing is why encode keys? enforcing a key to string seems to be reasonable
*be a string
alastairp
again, this may be a throwback to memcache
the key encoding stuff goes all the way back to 2016
_lucifer
yeah right, that function was never updated when we switched to redis
alastairp
but I do see some values in ensuring that we have ascii keys
however, they could stay as strings
_lucifer
agreed.
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
_lucifer
another thing I was thinking make the cache module a class
so that we can disable/enable encoding on a class level instead of passing it to each function
this would reduce the chance of mistakes, say we encoded while storing but forgot to decode while retrieving
alastairp
which is getting a bit silly, because by that point we're basically at the Redis() class again :)
_lucifer
yeah...
alastairp
let's think of this in terms of the current release that we're trying to get out.
string keys + sets + encoding values is a good start
_lucifer
one thing i noticed is that redis encodes values to bytes while storing and does not decode them while retrieving
alastairp
that'll allow us to get a new BU version out soon, and update CB + LB rate limiting
_lucifer
yeah sounds good
alastairp
interesting. I see that by default it returns bytes
but r = Redis(decode_responses=True) will make it convert to string
of course, we can't use that if we still use msgpack
_lucifer: tomorrow is a holiday here and I'll likely be taking most of the day off. what is your plan?
_lucifer
i'll be around
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)
_lucifer
the cache works seems to have a lot of potential for improvements
let's finalize everything on the cache front
also, did you push the tests branch to unify unit tests and integration tests? let's discuss that if time permits after cache.
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
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
_lucifer
cool, makes sense.
i should still add the encoding parameter to it, right?
alastairp
yes
just added a review
_lucifer
👍
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
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
_lucifer
I'll take of that PR. no worries.
should we do special casing now that we have sets?
I'm not sure why we specifically need sets for this
isn't it just an http request with no entity id?
_lucifer
yes
but there are filters like sort, type etc that can still be passed
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
when we expire keys for ws_cache{entity_id}, we also expire all keys in ws_cache_blah
_lucifer
yes
alastairp
just a question - do you know why userid is in the cache key?
_lucifer
no, it was that way before me
alastairp
ah, I guess it's None for an anonymous user
_lucifer
yes that's right
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
alastairp
that user info is who wrote the review, no?
JOIN "user" ON review.user_id = "user".id
_lucifer
ah yes right
alastairp
this also reminds me - we should open a ticket for this
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
it'd be safer to just not return this information
oh. maybe we need it to show the gravatar. that's annoying
never mind.
are you good for the next few days, then? I need to go
_lucifer
i'll need to look deeper to see if we can avoid it
yeah sure.
Happy holidays!
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?
yvanzo
reosarevok: no, quite the contrary
ruaok
_lucifer: did this also include the consul changes?
jasondk: hit me!
_lucifer
ruaok, yes.
ruaok
also the container startup stuff?
ruaok gets really optimistic now
_lucifer
no, not that.
ruaok
oh, well something to look forward to.
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.
_lucifer
that PR is still open but I work with alastairp on it next week,
