maritz / nohm

node.js object relations mapper (orm) for redis
http://maritz.github.io/nohm/
MIT License
500 stars 64 forks source link

Foo.load(id), Foo.find, Foo.remove(id) #18

Closed dvv closed 13 years ago

dvv commented 13 years ago

Hi!

The methods in the subj would be good to have as model static methods:

Foo.load(1, function(err, foo1) { foo1 loaded }) instead of, or in addition to var foo1 = new Foo; foo1.load(1, function(err) { foo1 loaded})

Foo.remove(1, function(err) { foo#1 deleted }) instead of, or in addition to var foo1 = new Foo; foo1.load(1, function(err) { foo1.remove(1), function(err) { foo#1 deleted }) })

etc.

What do you think?

TIA, --Vladimir

maritz commented 13 years ago

Hi,

I know what you mean and I've had it implemented at some point. However it required some hacky stuff and wasn't as easy as your examples. Remove should be rather easy, but load/find would require some work.

The problem would be that we'd have to create a new instance inside the load/find call (if called statically) and then somehow make it available inside the callback. This could go hand-in-hand with setting this inside the callbacks to the model itself.

I think I'll do it. It's probably not going to be this week though.

dvv commented 13 years ago

Thanks for a quick answer!

You mentioned setting callback contexts to model, and this is virtue, as it could allow to reduce amount of closures i believe.

Apart from that, what do you think on using a rather strong nonce-s for autogenerated ids, to reduce race conditions and ease future sharding (if any). E.g.

//
// simple nonce generator
//
function rnd() {
    return Math.floor(Math.random() * 1e9).toString(36);
}
function nonce() {
    return (Date.now() & 0x7fff).toString(36) + rnd() + rnd() + rnd();
}

is sufficiently simple, unpredictable yet fast, imho

maritz commented 13 years ago

I'm not sure what exactly you mean. Do you mean the temporary ids that are given to objects before the unique check or do you mean the real ids?

The real ids are just counted upwards. As to the temp ids: I'm not sure what I was thinking there. The ids can easily just be the current unix timestamp negated (*-1). The only circumstance in which they stay more than a few ms is when the redis server or node application crashes right after creating the temp id. With using negated timestamps there is no way they can collide (with other temps or real ids) and searching for them to do database cleanup should be rather easy. I'll create a ticket for changing that, so I don't forget about that the 50th time. :D

Regarding contexts to the instance: Yes, there is another issue open regarding exactly this. I haven't had the time to implement it yet, but plan on doing so this week.

edit: Well, actually just unix timestamps they can collide very easily with other temp ids. I'll use

(unix timestamp+random.toString())*-1

edit2: something funny about the temp ids: I forgot to make an issue for that AGAIN. Oh well, fixed now. Will push as soon as I have the rest of this ticket done.

dvv commented 13 years ago

I meant real ids. If we make them by counting, we must use a locking incr on a separate redis key, and this key is not compatible to making a cluster of redis server instances. If we assign a nonce, we can avoid taking care of these issues.

maritz commented 13 years ago

Hm, I've not yet done anything with clusters/sharding on redis and really don't know anything about it.

I'm guessing the problem is that we do an incr to one redis noe and assume the new id is the return while on another redis node the same id is returned to another request? Is that right? How exactly does a nonce solve this problem?

I'll do some reading up on redis cluster. (is it out already? I thought it was still going to take a while)

dvv commented 13 years ago

You're right in your guess. A nonce is unpredictable and sufficiently unique -- consider UUIDs. If you assign UUID to a record id, you can be almost 100% sure there was no, is no, and will be no such keyed record.

Am slightly off redis development news, but we could be prepared and have support ootb ;)

maritz commented 13 years ago

Yes, I agree. I like the simple ids for easy APIs though. I think it would be good to switch to nonces per default and give the user an option to keep ids of some models by just counting up. This way if he knows he doesn't need the shardable ids but wants simple ids, he can have it.

Does that sound good?

dvv commented 13 years ago

Perfect. Will be waiting for updates. TIA

maritz commented 13 years ago

Short forms for load, find, save and remove are implemented.

maritz commented 13 years ago

Hm... @dvv: Any idea how I would write a unit test for nonce ids? :D

maritz commented 13 years ago

Anyways, it's finally implemented: e35b6de899d

dvv commented 13 years ago

Great! I slightly drifted away from storage solutions, but will check the stuff asap. Thanks!