Closed dexonsmith closed 12 years ago
Still haven't had a chance to review this. Sorry @duncanexonsmith for the crazy slowness on my end.
Took my first real pass on this this afternoon. Lots to look through in more detail, but first impressions: this is amazingly awesome work!
@duncanexonsmith I've pushed the new version (6-dead-stones
) live, but have not made it "production" yet -- I've got a way to go before I'm ready to do that. But, that said, you can play with it against the live database here:
http://6-dead-stones.davepeck-go.appspot.com/
I've also added you as a Developer
to the app engine app (davepeck-go
) itself, so if you want to play with things you can look at logs, etc. for the service. Obviously please don't go deleting user data. ;-)
Cheers, Dave
@davepeck Sounds great. I'm glad there's a way to push it live without making it production. I'm very busy with work right now, but I hope to get a chance some time this week to have a look.
@davepeck I was playing through a 19x19 game on 6-dead-stones, and I hit a surprising error around move 257 in "/service/make-this-move/":
RequestTooLargeError: The request to API call datastore_v3.Put() was too large.
Have you ever seen this before? If you want to have a look, run SELECT * WHERE ANCESTOR IS 'agtkYXZlcGVjay1nb3INCxIER2FtZRjhl4wBDA'
.
current_state
is under 4K. My best guess is that the addition of the owners
array in the state makes Game.history
too large, but that seems strange. Any other ideas?
EDIT: Yes, that's it, actually. There's apparently a 1MB limit per put. 4K*256 == 1MB
, so along with the other data, my <4K
hits the limit. See http://stackoverflow.com/a/3068371
EDIT2: This shouldn't be too hard to fix. GameState.owners
is only needed for the final state. It can probably be left uninitialized for all the states in Game.history
.
@davepeck I did a quick check of 84e670323270a5b269c5653d47d07f4301bedd57 (local 10 move game) and didn't seem to break things. Let me know when you've had a chance to push it live (maybe 7-dead-stones
?) and I'll start a new test game.
Just a note about a1b3ffda1c443ee5aeb23ff5858d3d1c1f3a08f3 and eaf3467798a66465d8462f3fb31e8537e63fe136: these are fixes for issue #7.
...and then my entire life went insane. I'm still here, @duncanexonsmith -- just got snowed under by my day job, a tiny little app I maintain called Cloak -- but slowly digging out from underneath.
Work happens... I figured you'd come back eventually.
What! @duncanexonsmith -- bet you thought I'd disappeared. But I just merged this and pushed the entire thing live. It's amazing work and DESERVED TO BE USED rather than sit around languishing in my push inbox.
Thanks so much for this major new advancement to the site. In addition to life insanity, this was a big enough set of changes that I felt I had to really pour over the commits. But they look great; I really like how you kept the same interaction style throughout.
I know you mentioned replacing the temporary graphics -- well, I haven't had a chance, but I have a pretty good feel of what I'd like 'em to look like -- so, stay tuned for that. In the mean time... thanks again.
Cheers, Dave
Thanks, @davepeck.
I've been travelling, but finally had a chance to try it out live just now... and I've already found a bug (forgot to add komi!). Have a look at #8 when you get a chance... it's just a one-liner!
I've tested this with my own SDK, but you should test it too before putting it live. A few things to note: