Decouple identity map from object instantiation #286

Merged
merged 5 commits into from Jul 11, 2012

Conversation

Projects
None yet
2 participants
Collaborator

stve commented Jul 11, 2012

Modifications to Twitter::Base and Twitter::Identity to decouple the identity map from object instantiation which should fix #285

Owner

sferik commented on 770b4f4 Jul 11, 2012

This looks good to me. Is this branch ready to merge into master and ship as version 3.2.0 or is there more work to be done?

Collaborator

stve replied Jul 11, 2012

A little more work to be done I think, there's some IdentityMap code in Twitter::Identity that might also be impacting things on TweetStream since Twitter::Status inherits from it. Should have those changes done soon.

Collaborator

stve replied Jul 11, 2012

Can you eyeball 5058160? Basically just redefining Twitter::Identity.store to use of the id for identity map lookups and allowing instantiation without storing.

Owner

sferik replied Jul 11, 2012

There's also the matter of the Twitter::Status class (among others) generating "child" objects using fetch_or_create. This means that even if you initialize a Twitter::Status object using new (so it's not in the identity map), the associated Twitter::User that gets initialized if Twitter::Status#user is called will be put into the identity map.

I'm not sure how to solve this.

One idea is to add a class method called something like reset_identity_map!, which would empty the identity map for that class of objects. This could be used to solve the ballooning memory problem in tweetstream but is pretty inelegant.

Another idea is to have an instance variable that knows whether the instance was initialized with new or with fetch_or_create and then would generate child objects using the same method. This seems better but still not perfect.

Yet another solution is to have a global setting for the identity map. In this scenario, we wouldn't need a separate create method. You could simply call new and it would check the global to see whether the identity map is enabled or disabled. As a general rule, globals are bad, but there's already a module for global settings, so what's one more? This solution seems to map well to what users actually want to do ("I want to be able to turn off the identity map.") and gets rid of the distinction between new and create. Of course, it would require renaming fetch_or_create back to fetch_or_new. 😉 (The method would simply skip the fetch attempt if the identity map was disabled.)

What do you think? There are probably other solutions I'm not thinking of so don't constrain your thought to the ideas I've outlined above.

Collaborator

stve replied Jul 11, 2012

It would seem like an global toggle might be necessary. If that's the case, would it make sense to move the IdentityMap to a global too, that would make it more easily adaptable as well.

I had thought about providing methods to flush the identity map as well but having to explicitly do so doesn't seem like a good option. So, if we boil this down:

  1. global setting for enable/disable identity map
  2. global setting for accessing the identity map
  3. removal of create and reverting to fetch_or_new and new with a check on the identity map configuration before storing in the identity map

Did I missing anything?

Owner

sferik replied Jul 11, 2012

Nope, I think that's everything. Exposing the IdentityMap as a configurable seems like a good idea. Then someone could easily disable the Identity Map by doing:

Twitter.identity_map = false

Or, they could specify a custom IdentityMap:

Twitter.identity_map = My::Custom::SqliteIdentityMap

Then, instead of calling IdentityMap.new directly in Twitter::Base, you could use the accessor instead:

Twitter.identity_map.new if Twitter.identity_map

The default identity map would obviously remain Twitter::IdentityMap a.k.a. Hash.

Can you explain why you're calling super here?

Collaborator

stve replied Jul 11, 2012

Hmm, seems superflous actually. Since an object can't be instantiated unless it has an id, there's probably not a situation where the left side of the expression wouldn't evaluate.

Owner

sferik replied Jul 11, 2012

Yeah, that was confusing to me but I thought I might be missing something. Other than that, this commit looks good.

Owner

sferik commented Jul 11, 2012

I'm happy to pull this now. The code seems like a net improvement and is worthy of being merged into master.

However, it leaves the issue of "child" objects (discussed here) unresolved. I'd like to figure this out before releasing version 3.2.0, but it doesn't necessarily need to be part of this pull request.

Do you want me to pull now or wait for that issue to be resolved?

Collaborator

stve commented Jul 11, 2012

The implementation we've been discussing deviates quite a bit from the original intent of this pull request. It probably makes sense to merge in before it snowballs into something much larger. I'll make a new branch for the new changes.

@sferik sferik added a commit that referenced this pull request Jul 11, 2012

@sferik sferik Merge pull request #286 from sferik/fetch_or_store
Decouple identity map from object instantiation
4c4ffde

@sferik sferik merged commit 4c4ffde into master Jul 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment