bolshakov / stoplight

:traffic_light: Traffic control for code.
http://bolshakov.github.io/stoplight/
MIT License
384 stars 40 forks source link

Use fewer Redis keys #18

Closed tfausak closed 10 years ago

tfausak commented 10 years ago

Fixes #11.

I also rejiggered the in-memory store to behave more like the Redis store. It can store an array at @data['failures'], which Redis can't do. So Redis has one key to track failures per stoplight.

:pager: @clifton @hadronzoo

tfausak commented 10 years ago

If we wanted to use only 4 keys, we'd have to store a JSON array at each value in stoplight:failures. That would require two Redis commands.

failures = @redis.hget('stoplight:failures', name)
failures = failures ? JSON.parse(failures) : []
failures.push(Failure.new(name, error))
@redis.hset('stoplight:failures', name, failures.to_json
hadronzoo commented 10 years ago

A belated LGTM

camdez commented 10 years ago

Unless I'm missing something obvious, I think things are completely broken post-merge. e.g.:

> l = Stoplight::Light.new('foo') { puts 'blah' }
=> #<Stoplight::Light:0x007f98cbca3c88 @allowed_errors=[], @code=#<Proc:0x007f98cbca3b48@(irb):1>, @name="foo">
> l.run
blah
=> nil
irb(main):003:0> Stoplight.names
=> []

Looks like set_state is never called.

tfausak commented 10 years ago

I must've messed up the merge. My bad.

camdez commented 10 years ago

Actually, I don't think that's it at all, I think there's just a design flaw. When a light runs we call sync_settings, which saves the threshold to Redis. That threshold key used to be what names found to report the existence of a light (well, one which did not have other keys like attempts or failures).

Now that names is based on the states hash we'll also have to push a state in on every request, right? (In addition to the threshold.) That doesn't sound great.

tfausak commented 10 years ago

Ah, right. So here's the deal: I wasn't sure which key to use to signify that a light exists. For instance, DataStore::Memory#names could be implemented like this:

def names
  [all_attempts, all_states, all_thresholds].map(&:keys).uniq
end

I guessed (wrong) in 731982c237955340ed2da33a8ee87f6b60598c55.

camdez commented 10 years ago

Gotcha. So, back to threshold?

tfausak commented 10 years ago

I think so. We should also change the spec so it catches this.

camdez commented 10 years ago

Yeah, I was just commenting to Steffy about that. Beautiful test suite, but it let this sneak through. :)

It has to be threshold as is. That's the only thing we push in every time a light is run.

camdez commented 10 years ago

Ok, yeah, changing to threshold fixes it for the in-memory data store. I'll fix for Redis too and push up a commit.

tfausak commented 10 years ago

Beat you to the punch :smile:

camdez commented 10 years ago

Haha, indeed. But the good news is my other stuff (#24) works now (with these changes merged in).