sasa1977 / con_cache

ets based key/value cache with row level isolated writes and ttl support
MIT License
910 stars 71 forks source link

Support bag and duplicate bag #26

Closed fcevado closed 8 years ago

fcevado commented 8 years ago

issue #25

fcevado commented 8 years ago

I didn't update the docs and readme, firsts I wanted to know if there isn't anything missing. I'll do that in this same pr, as soon as it is approved.

sasa1977 commented 8 years ago

This looks good and mostly consistent with behaviour of raw ets tables.

The only difference I see is that retrieval operations in ConCache return a single value or a list of values, whereas ets functions always return a list of values. I'm not 100% sure if ConCache should be changed, and it would be a breaking change anyway, so let's leave that as it is for now.

I think the following is still missing:

Thanks for doing this!

fcevado commented 8 years ago

The only difference I see is that retrieval operations in ConCache return a single value or a list of values, whereas ets functions always return a list of values. I'm not 100% sure if ConCache should be changed, and it would be a breaking change anyway, so let's leave that as it is for now.

I totally agree about that, I think a change like that to fit more a change to a 1.0 version, since it would break applications already using ConCache.

About the necessary changes, I'll try to do that during the week, I dont have too much free time but I'll do it.

fcevado commented 8 years ago

@sasa1977 I started to apply the changes, and I saw that insert_new uses update to execute. If I validate the ets type on update or dirty_update, insert_new wont be supported anymore. Should I change insert_new or include it on non-supported functions for bag types?

You said that I should raise on wrong ets type. I thought about return only the tuple {:error, :ets_type_not_valid}. Should I return the error or raise?

sasa1977 commented 8 years ago

Looking at :ets.insert_new I see that it works for bag types, which makes sense to me as well. So we should support insert_new for bags.

There is in fact no need for insert_new to rely on update, since there's ETS native version of insert_new. Luckily, I've actually made that change in a 0.12.0 branch, but I still didn't decide on merging that branch. I think if you cherry pick this commit into your own branch, the problem would be solved.

You said that I should raise on wrong ets type. I thought about return only the tuple {:error, :ets_type_not_valid}. Should I return the error or raise?

Error tuples are usually returned if the caller can do something meaningful with them. I believe this is not the case here. Moreover, :ets raises in such situations, so I think that would be consistent behaviour. I'd say raising an ArgumentError with a helpful message would be a good approach here.

fcevado commented 8 years ago

I think that's it. I read the docs a few times and I think that were it needs to be mentioned, I put disclaimers about bag and duplicate_bag.

Do you want that I up the version and update changelog?

sasa1977 commented 8 years ago

Thanks for contributing!