#metabrainz

/

      • MRiddickW joined the channel
      • sumedh joined the channel
      • sumedh has quit
      • BrainzGit
        [listenbrainz-server] amCap1712 opened pull request #1393 (master…gh): Check for CI env variable https://github.com/metabrainz/listenbrainz-serv...
      • _lucifer
        alastairp: what do you think is a good way to handle the above issue in the test.sh script. The test.sh script currently exits with a ) exit code even if the tests fails. I was thinking to use `set -e` but that doesn't work because we use functions that return non-zero values eg: `is_unit_db_running`. Other option is to store the exit code of the command that ran tests in test.sh and then return it at the end of the script.
      • This looks fine to me. Do you think there's a better a solution?
      • alastairp
        _lucifer: hi, I saw that message last week
      • does test.sh run multiple tests in the same job? (e.g. frontend runs 3 things in a row, right?
      • so if the first one fails, will the other two run?
      • _lucifer
        No, we call it thrice to do three different things.
      • alastairp
        oh, that's great
      • and do we try and do cleanup at the end of the job?
      • _lucifer
        test.sh handles that
      • alastairp
        even if you set -e ?
      • _lucifer
        actually set -e doesn't work here as i mentioned above. my intent was set -e then even if cleanup doesn't happen in the CI its fine because the container will just go away.
      • alastairp
        yeah, right
      • _lucifer
        when is_unit_db_running or any other function we define in the script returns a non zero value it exits.
      • so we'll have to handle it manually i think.
      • alastairp
        I was going to suggest one option would be to use exec, that makes docker-compose take over the scrit, and its return value will be the return value
      • but that means you won't be able to do a cleanup after
      • _lucifer
        that's an issue when we run locally.
      • alastairp
        yeah
      • so, you were thinking of catching the return value of docker-compose, and then returning that at the very end of the script?
      • _lucifer
        yes
      • one issue is that we lose the return value of the cleanup but i see we are not using it anyway, we just exit 0.
      • alastairp
        I don't see a problem with not having the return value of the cleanup
      • _lucifer
        cool, let's do that then catch the return value and return it at end.
      • alastairp
        even though it's a bit annoying I still think it's the better way to go, using test.sh in the action
      • _lucifer
        yeah, also we'll probably replace it with a test.py soon which should be easier to work with IMO.
      • outsidecontext
        Nena should have sung about "100 Luftballons", then it wouldn't fool Picard's guess track number from filename :(
      • ruaok
        time to build a time machine, clearly.
      • _lucifer
      • BrainzGit
        [bookbrainz-site] MonkeyDo merged pull request #577 (master…feature/mailer): Adding mailing service to the codebase https://github.com/bookbrainz/bookbrainz-site/p...
      • [bookbrainz-site] MonkeyDo merged pull request #590 (master…autocomplete-search-limit): Autocomplete search limit https://github.com/bookbrainz/bookbrainz-site/p...
      • travis-ci joined the channel
      • travis-ci
        Project bookbrainz-site build #3861: passed in 4 min 30 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...
      • travis-ci has left the channel
      • travis-ci joined the channel
      • Project bookbrainz-site build #3862: passed in 6 min 40 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...
      • travis-ci has left the channel
      • ruaok
        zas: ping
      • zas
        pong
      • ruaok
        hey. those 5 machines that make up the old spark cluster are ready to be decommissioned. unless you have a use for them?
      • if not, I'll cancel them now.
      • zas
        I don't use them, so I guess you can proceed
      • ruaok
        do you have any need for them?
      • since we have them? I mean, they were previously used server with no setup cost, so we don't loose much if we dont use them.
      • MRiddickW has quit
      • zas
        what are the specs?
      • ruaok
        8 core, 32GB RAM, 200GB disk
      • zas
        those are named lb-* in hetzner robot right?
      • ruaok
        yes
      • zas
        I could use them I think, to migrate a part of our installation to vswitch and 20.04
      • ruaok
        all of them?
      • zas
        I guess one or two would be enough to experiment
      • let's say 2, for convenience
      • ruaok
        ok, I'll ditch the rest.
      • I'll leave you to get rid of them once we no longer need them.
      • zas
        ok
      • I can trash everything on them I guess, so I'll do a full reinstall from scratch
      • BrainzGit
        [bookbrainz-site] MonkeyDo merged pull request #593 (master…merge-queue-buttons): FIX(BB-606): Buttons overlapping in merge entity section https://github.com/bookbrainz/bookbrainz-site/p...
      • ruaok
        yes
      • three servers cancelled.
      • the remaining two are now called zas-test-0 and zas-test-1
      • travis-ci joined the channel
      • travis-ci
        Project bookbrainz-site build #3863: passed in 4 min 26 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...
      • travis-ci has left the channel
      • ruaok
      • do you know where that function is called from? I can't find the right place...
      • _lucifer
        ruaok: i see its being called in get_or_create in the same file which is then called from listenbrainz/webserver/login/provider.py
      • iliekcomputers
      • this is the endpoint
      • BrainzGit
        [bookbrainz-site] MonkeyDo merged pull request #601 (master…browser-compatibility): FIX(BB-615): Copy/Paste annotation text in FireFox <= 60 https://github.com/bookbrainz/bookbrainz-site/p...
      • travis-ci joined the channel
      • travis-ci
        Project bookbrainz-site build #3864: failed in 4 min 48 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...
      • travis-ci has left the channel
      • travis-ci joined the channel
      • Project bookbrainz-site build #3864: passed in 4 min 8 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...
      • travis-ci has left the channel
      • ruaok
        _lucifer & iliekcomputers: I see now -- thanks!
      • in get_or_create() I would like to set default mint_ts and max_ts timestamps for a user into redis, but only if the user is new. but in the db.user.py module is the wrong place to do it (logically, and also importing the timescale listenstore yields an import circular dependency. so it should be done in listenbrainz/webserver/login/provider.py
      • _lucifer
        ruaok, python packages set up on new cluster. dataframes generated successfully. there is an incompatitble change wrt to schema in spark 3.* which needs to be fixed before recommendations start working. currently working on that.
      • ruaok
        but, there I can't tell if the user was created or not -- could return a tuple of (user, created), but our tests call this function 94 times, which makes the PR much larger and more noise in our codebase.
      • thoughts for how to best to this iliekcomputers _lucifer alastairp ?
      • _lucifer: thanks. this is all perfect timing, I hope to get recommendations working again this week.
      • alastairp
        ruaok: sorry, was in a call and didn't see your first question
      • ruaok
        no worries.
      • alastairp
        in django, their version of get_or_create returns a tuple (object, created), where you can check this
      • but as you point out, the method is already used in a bunch of places
      • ruaok
        94 more changes to the PR would suck.
      • alastairp
        oh, you already said that. missed that part of the line
      • ruaok
        I could just add another get_by_mb_row_id call to check to see if the users exists. if it is fetched, the next call should effectively be cached in PG and return quickly.
      • _lucifer
        if the only issue is that, we could do it in a separate PR and merge that first.
      • alastairp
        just loading code up now
      • ruaok
        _lucifer: yeah, I'd honestly prefer to do it in different way and not clutter up our tests more than needed.
      • alastairp
        (there are also 34 instances of `create` in tests which for some reason don't use get_or_create :|)
      • ruaok: hmmm
      • ruaok
        indeed.
      • alastairp
        if you did it in provider.get_user, you could get, if it's none set a sentinel, do get or create, then if it was created, set the redis stuff
      • that's what you suggested?
      • ruaok
        yeah
      • alastairp
        yea, fine by me
      • ruaok
        k, less mess.
      • alastairp
        one potential problem there is that this is only used from the web login flow (right?). if we need this data in redis for an integration test, we might have to create it manually, which then means we might need 2 ways of making the same data
      • ruaok
        alastairp: I dont think we need an integration test -- I'll add a regular one and should be fine.
      • meh, we have no tests at all for testing the login process.
      • _lucifer
        a good time to do that when we do account migration
      • ruaok
        agreed.
      • alastairp
        ruaok: right, I didn't specifically mean that we need an integration test for this. I mean, if you have an integration test that requires reading these values from redis, how are you going to set them?
      • ruaok
        by calling the cache.put -- afterall I am only inserting (0,0)
      • the idea is this: if a user creates an account and then goes to their listens page, that would causes us to do an INDEX scan on the whole DB, with the guarntee of not finding anything.
      • but inserting (0,0) for the timestamps, means that the timescale listenstore knowns there are no listens and does not query.
      • alastairp
        ok, good
      • _lucifer
        these keys will not be expiring automatically?
      • CatQuest has quit
      • CatQuest joined the channel
      • CatQuest has quit
      • CatQuest joined the channel
      • ruaok
        no, they are now intended to be in redis at all times without expiry. we now update the keys when the DB values change.
      • in fact this is the only reason why listen counts were ever accurate in the first place, lol. ;)
      • _lucifer
        cool, you might want to pass `time=0` manually then
      • we'll be updating BU sometime soon so that the caller has to always specify a timeout. `time=0` if you do not want expiry.
      • ruaok
        oh, that's important.
      • _lucifer
        New recommendations generated.
      • ruaok
        :D
      • alastairp
        this is a good place to remind us that we had this open ticket about separating the redis interface in BU into cache (epheremal, may disappear) and data (need to keep around permanently), and if we should have 2 different interfaces for that task [not suggesting we do it now, let's just use cache, but something to keep in mind for future improvements]
      • ruaok
      • not terribly useful.
      • _lucifer
        yeah, rtd is not setup completely for BU yet.
      • alastairp
        oops, sorry about that
      • _lucifer
        I think one interface is good enough. we can call setex methods to cache and call it a day. after all the underlying implementation is the same.
      • its unlikely we'd use different things for cache and an in memory db. and we already have h* methods in cache and will be having set methods soon.
      • ruaok
      • is that correct for setting a 0 expiry then?
      • _lucifer
        yes. you can do `time=0` for more clarity if you want
      • alastairp
        _lucifer: unfortuantely the version of BU ruaok is using will still have it called expireat :(
      • _lucifer
        also, there is a `namespace` keyword arg, which you can pass `self.ns` to.
      • alastairp
        that is: cache.set(redis_user_listen_count+username, 0, 0, namespace=self.ns)
      • ruaok
        I think I don't need to pass in a namespace at all, I think that already setup.
      • `listenbrainz:listenbrainz.ts.lb_test_1`
      • _lucifer
        alastairp, no i think it'll be `time` for him. we haven't made a release after changing to `expirein`.
      • but let me check to confirm.
      • alastairp
        _lucifer: oh, I got them around the wrong way then
      • _lucifer
      • ruaok
        so time=0 is correct?
      • _lucifer
        yes.
      • alastairp, https://github.com/metabrainz/listenbrainz-serv... is updated with the comment. should i merge?
      • alastairp
        (sorry for the confision)