search

sasa1977 / con_cache

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

TTL Check should have a default value or produce a warning #41

Closed Secretmapper closed 6 years ago

Secretmapper commented 7 years ago

I think it's a bit non-intuitive to not have a default TTL Check.

If a default one is unwieldy (as it should be based on an application's requirements) I think a more prominent warning on the console on development mode would be great

sasa1977 commented 7 years ago

Hi,

Thanks for the proposal. I agree that it would be better to force the client to explicitly choose the expiry approach. Perhaps something like this:

ConCache.start_link(ttl: false) # no expiry
ConCache.start_link(ttl: time_in_ms, ttl_check: time_in_ms) # ttl expiry

In the case where ttl option is not provided the cache would still start (with no ttl expiry), but log a warning. Then, in the following version, the cache would refuse to start unless ttl option is provided.

Finally, I think we should error (i.e. refuse to start) in the following cases:

# error because ttl_check is not provided
ConCache.start_link(ttl: time_in_ms)

# error because ttl is not specified
ConCache.start_link(ttl_check: time_in_ms)

# error because ttl is set to false
ConCache.start_link(ttl: false, ttl_check: time_in_ms)

What do you think about this?

Secretmapper commented 7 years ago

Yes I agree with all those points! I think having a no expiry flag (ttl: false), plus warning then eventual error covers all the possible cases, and this should improve it a bit!

sasa1977 commented 7 years ago

OK, in that case the API is agreed. Do you want to give it a try yourself? If not, I can do it as well, but I can tell you that it's probably not going to happen in the next month, because of some other engagements.

sasa1977 commented 6 years ago

Solved with #42 and published to hex.