| 01:08.34 | *** join/#storm sidnei (~sidnei@200-102-96-212.cslce701.dsl.brasiltelecom.net.br) |
| 03:02.31 | *** join/#storm maxb_ (~maxb@jabberwock.vm.bytemark.co.uk) |
| 04:25.44 | *** join/#storm PascalHu` (~user@d60-65-84-128.col.wideopenwest.com) |
| 05:23.20 | *** join/#storm stub (~stub@ppp-58-8-212-28.revip2.asianet.co.th) |
| 05:23.20 | *** join/#storm stub (~stub@canonical/launchpad/stub) |
| 08:00.15 | *** join/#storm danilos (~danilo@canonical/launchpad/danilos) |
| 08:07.36 | *** join/#storm zmijunkie (~sascha@i53873CBB.versanet.de) |
| 08:10.04 | *** join/#storm zmijunkie (~sascha@i53873CBB.versanet.de) |
| 08:15.26 | *** join/#storm bigjools (~quassel@82-71-93-254.dsl.in-addr.zen.co.uk) |
| 08:15.26 | *** join/#storm bigjools (~quassel@canonical/launchpad/bigjools) |
| 10:15.54 | *** join/#storm jkakar (~jkakar@46.Red-79-155-138.dynamicIP.rima-tde.net) |
| 10:35.21 | *** join/#storm danilos (~danilo@cable-94-189-240-33.dynamic.sbb.rs) |
| 10:35.21 | *** join/#storm danilos (~danilo@canonical/launchpad/danilos) |
| 11:12.10 | *** join/#storm zmijunkie_ (~sascha@i59F5DC08.versanet.de) |
| 11:29.39 | *** join/#storm stub (~stub@canonical/launchpad/stub) |
| 11:48.57 | *** join/#storm sidnei (~sidnei@189.30.217.33) |
| 12:02.11 | *** join/#storm zmijunkie (~sascha@i59F5DC08.versanet.de) |
| 12:04.06 | *** join/#storm artista_frustrad (~artista_f@201-41-172-10.ctaje701.dsl.brasiltelecom.net.br) |
| 12:07.53 | *** join/#storm webmaven (~webmaven@c-68-35-122-81.hsd1.nm.comcast.net) |
| 12:08.36 | *** join/#storm zmijunkie (~sascha@i59F5DC08.versanet.de) |
| 12:57.14 | *** join/#storm zmijunkie_ (~sascha@i53870FD5.versanet.de) |
| 14:15.33 | *** join/#storm fcorrea (~fcorrea@187.3.31.9) |
| 15:53.56 | *** join/#storm zmijunkie (~sascha@i53870FD5.versanet.de) |
| 15:58.12 | *** join/#storm yosafbridge (~yosafbrid@li14-39.members.linode.com) |
| 16:03.01 | *** join/#storm magmatt (~matt@74-92-245-181-Utah.hfc.comcastbusiness.net) |
| 16:38.11 | magmatt | How do you retry adding an object to a store that caused an exception on its first flush? http://pastebin.com/KkdXcEZS |
| 16:40.00 | magmatt | here's my output from running that: http://pastebin.com/3YT2vxMf |
| 17:17.59 | *** join/#storm sidnei (~sidnei@189.30.217.33) |
| 17:20.48 | magmatt | What should be on line 33 to make the assertions pass? http://pastie.org/1205786 |
| 17:29.55 | marienz | eyes those code snippets |
| 17:32.20 | magmatt | marienz, start at the last one if you'd like |
| 17:32.38 | marienz | that does look more convenient |
| 17:35.21 | marienz | hmm, this does feel kinda broken |
| 17:38.49 | marienz | if I'm reading this right _flush_one is popping the "pending add" from the object's state before actually running any queries |
| 17:39.07 | marienz | so if I understand correctly storm considers the object as "in the db" after that failing flush(), even though it actually isn't |
| 17:40.07 | marienz | I don't know how intentional that is |
| 17:41.42 | *** join/#storm sidnei (~sidnei@189.30.217.33) |
| 17:43.17 | marienz | I can't actually find a bug on it though |
| 17:43.18 | marienz | hmm |
| 17:43.32 | marienz | sorry, don't th ink I can help with this one |
| 17:43.35 | marienz | think, too |
| 17:44.11 | magmatt | marienz, k, I'm looking at the code now... I see what you're saying |
| 17:44.19 | magmatt | shall I file a bug? |
| 17:44.54 | marienz | might be worth waiting a bit for someone more familiar with storm to show up and weigh in |
| 17:45.21 | magmatt | alright... I'm pretty sure people have run into this before |
| 17:45.27 | magmatt | it seems like a basic thing |
| 17:46.59 | keturn | I'm pretty sure I had the same question. And I think the only answer I got is "run the entire transaction again" |
| 17:48.21 | magmatt | how do you get the bad object out of the store, though? |
| 17:49.34 | magmatt | calling store.remove(user) will set up the next flush/commit to fail |
| 17:50.56 | marienz | I suspect what works is to just drop your reference to the invalid user and create a new one |
| 17:51.36 | marienz | I have to wonder what happens if popping the "pending" state after instead of before actually running queries helps |
| 17:51.45 | marienz | hmm, let's see what happens if I make that change and run the test suite :) |
| 17:52.08 | magmatt | :) |
| 17:53.53 | magmatt | k, this line does work (on line 33) |
| 17:53.53 | magmatt | user = store.add(User('bar')); store.flush() |
| 18:02.58 | marienz | hmm, the test suite passes with this change |
| 18:03.12 | magmatt | I just ran the test suite with this change ( http://ietherpad.com/KadaYI7yFh ), and things still pass. |
| 18:03.14 | magmatt | k |
| 18:03.22 | magmatt | but what does it buy? |
| 18:03.41 | marienz | still testing |
| 18:05.12 | marienz | magmatt: http://paste.pocoo.org/show/272373/ <-- works with my slightly patched storm. Still prodding at it |
| 18:07.46 | marienz | yeah this is still broken |
| 18:08.07 | marienz | without the remove/add pair there it doesn't actually run a second INSERT query |
| 18:08.46 | magmatt | what would you expect the code to look like? |
| 18:08.52 | magmatt | just user.name = u'bar' |
| 18:08.52 | marienz | ah, I think I know why |
| 18:08.53 | magmatt | ? |
| 18:08.55 | marienz | yes |
| 18:08.59 | marienz | and a flush |
| 18:09.01 | magmatt | k |
| 18:09.16 | marienz | that is: iiuc you need a flush or it won't know the id |
| 18:09.26 | marienz | the original insert failed, so the object doesn't have an id yet (it's not actually in the db yet) |
| 18:09.32 | magmatt | right |
| 18:09.57 | marienz | another simple change to store.py and it works that way |
| 18:10.03 | marienz | running testsuite |
| 18:10.24 | marienz | testsuite fails |
| 18:10.33 | magmatt | dang, what was your change? |
| 18:10.59 | marienz | http://paste.pocoo.org/show/272376/ |
| 18:11.15 | marienz | the bottom two hunks made it almost work and passed the testsuite |
| 18:11.24 | marienz | the top hunk is new and breaks the testsuite (one test) |
| 18:11.28 | marienz | debugging |
| 18:11.59 | marienz | FAIL: test_flushed_hook (tests.store.sqlite.SQLiteStoreTest) |
| 18:14.09 | marienz | ah, I think I see what I broke |
| 18:14.30 | magmatt | should we write a test to test the behavior we expect? |
| 18:14.34 | marienz | I'm unconditionally removing things from _dirty after calling _flush_one, and that breaks the case where flushing makes the object dirty |
| 18:14.38 | marienz | yes, we should |
| 18:14.46 | marienz | I wanted to see if this was as easy to fix as I thought first |
| 18:14.51 | magmatt | k |
| 18:14.53 | marienz | (I'm naughty and don't always do TDD) |
| 18:15.00 | magmatt | WHAT!? |
| 18:18.22 | marienz | http://paste.pocoo.org/show/272382/ |
| 18:18.42 | marienz | that one passes the testsuite and makes this work: |
| 18:19.01 | marienz | http://paste.pocoo.org/show/272384/ |
| 18:19.12 | marienz | now I just need to turn that into a proper testcase, I guess |
| 18:19.23 | magmatt | I can have a try at that if you'd like |
| 18:20.05 | marienz | it's not terribly hard :) |
| 18:20.12 | magmatt | okay |
| 18:20.29 | marienz | however I don't think any of the existing store tests use a unique index |
| 18:20.41 | magmatt | right... I saw that I'd need to make a new table |
| 18:21.01 | magmatt | or figure out some other way of making a flush fail |
| 18:21.05 | marienz | most of the interesting stuff is in tests/store/base.py, but that operates on tables that don't have the index you need to test this, and how to create that index is db-specific |
| 18:21.35 | marienz | so I'd be tempted to just make this an sqlite-specific test for now, instead of adding the necessary constraint to create_tables for all databases |
| 18:22.21 | magmatt | The unique index syntax is the same for at least postgres and sqlite |
| 18:22.24 | magmatt | I'm not sure about mysql |
| 18:22.32 | marienz | I am not stellar at sql |
| 18:23.00 | magmatt | it looks like it's the same for mysql |
| 18:23.28 | marienz | also, I don't have the databases around to actually run the store tests for anything other than sqlite |
| 18:23.55 | magmatt | does anyone else in here? I've got postgres and I think I might have mysql running somewhere |
| 18:24.27 | marienz | I think it'd make sense to only add the tests for databases you can run them on for now, and then open a bug in launchpad so an actual storm dev can have a look |
| 18:24.40 | magmatt | k |
| 18:24.54 | marienz | the actual change here isn't db-specific anyway, or even specific to this particular constraint violation |
| 18:25.09 | marienz | it's really "deal with failed flushes better" |
| 18:25.16 | magmatt | right |
| 18:25.22 | magmatt | is there an easier way to make it fail? |
| 18:25.45 | marienz | I don't know off the top of my head |
| 18:26.37 | marienz | keep in mind I'm just a storm user (and happen to like including patches with bugs if at all possible) |
| 18:26.50 | magmatt | k |
| 18:27.05 | magmatt | so who's doing it? I'm happy to do it |
| 18:27.16 | marienz | go for it :) |
| 18:27.25 | magmatt | k, I'll paste here before I submit it |
| 18:27.30 | marienz | actually let me stuff this in a branch |
| 18:28.41 | marienz | lp:~marienz/storm/recover-from-failed-flush |
| 18:29.35 | marienz | (if you like you can now branch that, commit your tests, push it to lp:~you/storm/something, and then link that to the bug) |
| 18:29.50 | magmatt | sounds like a plan |
| 18:30.04 | marienz | (I don't know if you're a bzr/launchpad user) |
| 18:30.30 | magmatt | I'm not yet, but I will be :) I'm savvy with git, but I'll give bzr a try |
| 18:31.06 | magmatt | I appreciate your help a lot |
| 18:32.41 | marienz | no problem at all |
| 19:00.10 | magmatt | For whatever reason, tearDown in tests.store.base.StoreTest.tearDown never gets called in the postgres tests... |
| 19:00.22 | magmatt | probably my problem |
| 19:06.53 | marienz | pastebin your diff? |
| 19:18.11 | magmatt | http://paste.pocoo.org/show/272413/ |
| 19:19.02 | marienz | and the problem's what? |
| 19:19.22 | magmatt | all the PostgresStoreTests fail |
| 19:19.25 | magmatt | lemme get a traceback |
| 19:20.18 | *** join/#storm keppla (~keppla@i577B3704.versanet.de) |
| 19:20.46 | marienz | nit: self.assertRaises(IntegrityError, self.store.flush) |
| 19:20.52 | marienz | (no need for "def f") |
| 19:21.02 | magmatt | heh... right |
| 19:21.10 | magmatt | http://paste.pocoo.org/show/272415/ |
| 19:21.21 | magmatt | that's the traceback... so it's not cleaning up the table properly |
| 19:22.12 | magmatt | oops, I forgot to add uniq to the list of tables to drop |
| 19:22.16 | marienz | I don't see you having added a drop for... |
| 19:22.17 | marienz | yeah |
| 19:22.51 | marienz | looks like it should just be added to the list in StoreTest.drop_tables |
| 19:23.00 | magmatt | yup, that does the trick |
| 19:23.30 | marienz | nit: I'd do "uniqs = list(self.store.find(Uniq))" instead of "all = [x for x in self.store.find(Uniq)]" |
| 19:23.34 | marienz | ("all" is a builtin) |
| 19:23.59 | marienz | oh, and can you actually safely assert that the generated id is 2? |
| 19:24.09 | magmatt | yes, if the table is new for that test |
| 19:24.19 | marienz | ok |
| 19:24.20 | magmatt | I'm not certain on mysql, but I'll test it |
| 19:24.31 | marienz | that is: I don't know if all DBs actually guarantee that |
| 19:24.35 | marienz | but it'd make sense |
| 19:50.50 | *** join/#storm cody-somerville (~cody-some@ubuntu/member/somerville32) |
| 19:51.08 | cody-somerville | Hey. I found some weird behavior in storm but I'm not sure if its a bug or not. |
| 19:51.30 | therve | cody-somerville, shoot |
| 19:53.23 | cody-somerville | If you try to set a field to a timezone-naive datetime when the column type is TIMESTAMP WITH TIME ZONE then you get an error. However, if you set the field via a reference on another object then you're successfully able to commit. |
| 19:55.03 | cody-somerville | So either its a bug you can't do the former or whatever checks that occur when you do the former do not occur when changing an object via the latter which would be an even more serious bug I'm guessing. |
| 19:55.18 | therve | what do you mean, via a reference on another object? |
| 19:55.35 | therve | I suspect the latter is a bug |
| 19:56.41 | cody-somerville | Lets say you have object B which has a field that is a Reference to another object A. You can access object A via that field on Object B. |
| 19:57.38 | therve | so, B.a = Reference(B.a_id, A.id) |
| 19:58.12 | cody-somerville | so 'a.created_at = datetime.datetime.now(); store.commit()' fails but 'b.a.created_at = datetime.datetime.now(); store.commit()' is successful. |
| 19:58.46 | cody-somerville | If you grab the A object directly from the store after the latter you'll see that the field created_at was successfully changed and something along the way assumed the local timezone. |
| 19:59.11 | therve | yeah that's a bug |
| 19:59.44 | cody-somerville | Now the error that occurs when you make the change directly is caused by trying to compare a timezone-naive datetime and a timezone-aware datetime - it isn't Storm explicatively saying no this isn't allowed. |
| 19:59.58 | therve | that would make 2 bugs :) |
| 20:00.59 | therve | there is no reason you have a different code path in this situation |
| 20:01.04 | therve | unless I'm missing something |
| 20:02.50 | cody-somerville | I agree that thats a bug but ideally I think Storm should assume the local timezone if it gets a timezone-naive dt |
| 20:03.52 | therve | I'm not sure, assumptions are dangerous |
| 20:03.55 | cody-somerville | or do something to make dealing with a mix of timezone-naive and timezone-aware datetimes more convenient. |
| 20:05.05 | jkakar | cody-somerville: It's inconvenient in Python. I'm not sure what we could do to make it better that wouldn't be surprising. |
| 20:05.35 | cody-somerville | also, I've used storm.tz.tzlocal() to provide tz info to my datetime objects. That class isn't really documented so I hope thats not abusing it. |
| 20:06.36 | *** join/#storm ajmitch_ (~ajmitch@vps.ajmitch.net.nz) |
| 20:06.36 | *** join/#storm ajmitch_ (~ajmitch@ubuntu/member/ajmitch) |
| 20:06.39 | cody-somerville | jkakar, Postgresql will assume the local time zone (or another specific timezone if specified in psql config) if you don't supply tz info. |
| 20:08.03 | jkakar | cody-somerville: That's what I would expect. |
| 20:08.22 | magmatt | marienz: here's my diff (tested in sqlite and postgres -- mysql is being a pain) |
| 20:08.22 | magmatt | http://paste.pocoo.org/show/272438/ |
| 20:15.15 | magmatt | I lied... it wasn't tested -- going from git to bzr and to remote machines is a little confusing |
| 20:23.42 | magmatt | k, filed the bug |
| 20:25.40 | *** join/#storm chaoticmarienz (~marienz@freenode/staff/marienz) |
| 21:09.25 | *** part/#storm chaoticmarienz (~marienz@freenode/staff/marienz) |
| 21:10.05 | *** join/#storm wallyworld (~quassel@27-33-46-253.static.tpgi.com.au) |
| 21:43.05 | *** join/#storm fcorrea (~fcorrea@187.3.33.106) |
| 21:51.28 | cody-somerville | What does this mean?: storm.exceptions.CompileError: Don't know how to compile type Reference of <storm.references.Reference object at 0xe69f50> |
| 22:16.30 | jkakar | cody-somerville: It means you should replace the Object.reference with Object.reference_id in your query. |
| 22:17.02 | jkakar | cody-somerville: It's something lifeless has brought up, but I think there's been some disagreement about fixing it. I'm not quite sure of the details, though. |
| 22:17.12 | cody-somerville | Is it intentionally like that or is it just a missing feature? |
| 22:20.13 | jkakar | cody-somerville: I *think* it's just a missing feature, but as I said, I'm not sure of the details. It seems to be a problem triggered in (at least) one particular scenario, but not others... I think there's some contention about whether the potential scenario is something worth supporting. |
| 22:20.20 | jkakar | cody-somerville: What are you trying to do that's causing the issue? |
| 22:28.15 | cody-somerville | jkakar, store.find(Release, Release.milestone == store.find(LaunchpadProjectMilestone).any()).set(milestone=None) |
| 22:30.45 | jkakar | cody-somerville: Yeah, so try: store.find(Release, Release.milestone_id == store.find(LaunchpadProjectMilestone.id).any()).set(milestone_id=None) |
| 22:31.10 | jkakar | cody-somerville: In addition to working, it'll be faster, because it won't load a LaunchpadProjectMilestone instance, it'll just fetch the ID. |
| 22:31.27 | jkakar | cody-somerville: Assuming the milestone link on the Release object is called milestone_id, of course. |
| 22:31.32 | cody-somerville | nods. |
| 22:31.40 | cody-somerville | Thats what I've done, yup. |
| 22:33.36 | cody-somerville | (or pretty much, I realized I'm being passed a ResultSet and not a specific object so doing Release.milestone_id.is_in(resultset.values(LaunchpadProjectMilestone.id)).) |
| 22:34.37 | jkakar | cody-somerville: Yeah, basically the same thing. |
| 23:12.57 | *** join/#storm sidnei (~sidnei@201-66-131-217.cslce701.dsl.brasiltelecom.net.br) |