[02:45:51] firejail --quiet can be really confusing, since you can create firejails inside firejails I ended up 3 firejails deep while trying to figure out wtf was going on [19:24:16] anomie: I have been thinking about the issue of User objects in RevisionRecord. How about a UserIdentity interface, with just getUserId and getName? Perhaps plus getActorId. [19:24:46] We can use User objects for now. I just don't want to guarantee this in the interface of RevisionRecord. [19:27:54] anomie: what do you think? is that a usable idea? [19:40:41] DanielK_WMDE: Not a bad idea. We'd eventually want actor ID in there, yeah. There are a few more items that questionably could be in there: isAnon()/isLoggedIn() (both currently just compare ->getId() with 0), getEmail(), getRealName(). [19:43:57] DanielK_WMDE: It reminds me slightly of https://gerrit.wikimedia.org/r/#/c/188516/. The difference is that this proposed interface is even simpler, which oddly makes me less opposed to it for some reason. [19:59:41] anomie: getEmail etc shouldn't be in there - I explicitly want somethign that *just* covers the identity. [20:00:54] I like very narrow interfaces. You can't really have to many. They don't force us to split the implementation, they just give us a way for code to ask exactly for what it needs, and to guarantee exactly what it has. [20:02:27] anomie: if we had getMail etc in that interface, that would mean you can't call getUserIdentity without having joined against the user table, or loading on the fly - or have the object lazy load the address. I'd prefer a way to guarantee exactly the info I have in this context. which is id and name, for now. [20:03:30] DanielK_WMDE: Even with just user ID, name, and actor ID you're likely to have to select or load something. [20:03:47] Even with just user ID and name, you're likely to have to load the other. [20:11:05] anomie: that would be an argument to stick with getUserId and getUserName. [20:11:30] If you need more info, ask a UserInfoStore or whatever. [20:12:02] DanielK_WMDE: Where are you getting the ID and name in the first place to return from getUserId and getUserName? [20:12:11] from the revision table [20:12:18] ...or the log table [20:12:21] etc [20:12:25] Not for long you're not. [20:12:40] Actor table change means you get the actor ID from those tables. [20:12:47] right [20:13:10] that makes me feel we should just have getActorId on RevisionRecord [20:13:47] otherwise, you can't constrcut a RevisionRecord without loading things from the actor and user table [20:14:49] anomie: I want to avoid magic lazy loading if possible. i'm using it in a few places, but it's somethign i'm trying to get rid of, not something I promote. The only exception is loading the content blob - that I want to stay lazy. [20:15:42] though lazy loading would allow us a way out of this. [20:16:35] UserIdentity could be pre-filled based on a join, or initialize on demand. [20:17:20] I'm not super happy with that, but since we want to a) provide access to the user lame along with the revision and b) allow construction of revision record without joining against user, this is probably the only sensible option [20:32:28] DanielK_WMDE: There's probably not any reason not to join against actor for the construction of the revision record. That'll get you ID, name, and actor. [20:45:58] anomie: i just pushed a new patch set. I have started to add unit tests. i hope i can get full test coverage by the end of the week. [20:46:10] please let me know what else is missing for this to get merged. [20:46:34] we have to step up the pace a bit if we want to get the storage layer done be the end of the year (see also the plan i shared with you). [20:47:15] * anomie will get to it this afternoon or tomorrow [20:47:39] thank you [20:47:50] i'm off for today. nearly 10pm here [22:48:52] Krinkle: do you think you'll have time to look at https://gerrit.wikimedia.org/r/#/c/389793/ too? [22:57:35] Done