iron-io / iron_mq_ruby

Ruby library for IronMQ.
http://www.iron.io
BSD 2-Clause "Simplified" License
35 stars 17 forks source link

Logical bugs, other known issues #45

Closed featalion closed 11 years ago

featalion commented 11 years ago
queue = ironmq.queue "my_new_queue" # => #<IronMQ::Queue:... >
queue.size
Rest::HttpError: HTTP 404 Error. {"msg":"Queue not found"}
    from ...
  ..
queue = ironmq.queue "my_new_queue" # => #<IronMQ::Queue:...>
queue.post "my first message" # => #<IronMQ::ResponseBase:..., @code=200>
queue.id # => nil <-- wrong
queue.size # => 1 <-- queue info was reloaded
queue.id # => "512553723264140e863d6097" <-- now ID is right
queue.size # => 1
msg = queue.get # => #<IronMQ::Message:...>
msg.delete # => #<IronMQ::ResponseBase:..., @code=200>
queue.size # => 1 <-- must be 0 after message deletion
queue = ironmq.queue "my_queue" # => #<IronMQ::Queue:...>
queue.size # => 2
queue.post "my 3rd message" # => #<IronMQ::ResponseBase:..., @code=200>
queue.size # => 2

All this points are related to broken caching system for queue information.

The easiest way to fix this just make request to IronMQ API each time user needs any info, except queue's name. Handle HTTP 404 and return "default" value (0 for size, for example). It needs simple updates: drop current caching system and add default for HTTP 404. Requests for info are seems rare and mostly "technical" (not related to logic of application which uses IronMQ) this is may be a good choice.

Another possible solution is to make new caching system. I suggest to use additional layer which wraps all API (iron_core_ruby) calls in IronMQ::Client class. This layer must operate with all existing / new queues' info data. Next I place kind of example how it might be realized. Use case:

ironmq = IronMQ::Client.new(token: 'TOKEN', project_id: 'PROJECT_ID') # => #<IronMQ::Client:...>
queue = ironmq.queue "my_queue" # => #<IronMQ::Queue:...>
queue.size # => 7
queue.post "my message" # => #<IronMQ::ResponseBase:...>
queue.size # => 8

Possible cache realization scheme:

module IronMQ
  class Client < IronCore::Client
    attr_reader :info_cache
    ...
    def with_cache_response(queue_name, kind)
      response = nil
      method = caller[0][/`([^']*)'/, 1] # caller method name, "post", "delete", etc.
      yield response

      r = ResponseBase.new(response)
      self.call("#{method}_#{kind}", queue_name, r) if r.code == 200

      r # or we can return queue info cache instead of response, or both
    end

    def post_messages(queue_name, response)
      queue_info = @info_cache[queue_name]
      queue_info.size += response["ids"].size
      queue_info.total_messages += response["ids"].size
      queue_info
    end

    def delete_messages(queue_name, response)
      queue_info = @info_cache[queue_name]
      queue_info.size -= response["ids"].size
      queue_info
    end

    def info_queue(queue_name, response)
      @info_cache[queue_name] = OpenStruct.new(response)
    end

    # TODO: subscribers and other
  end
end

class Queue
  ...
  def info
    @client.info_cache[queue_name] || with_cache_request(queue_name, :queue) do |resp|
      resp = begin
        @client.parse_response(@client.get("#{path(options)}"))
      rescue
        self.create_queue_info_structure(queue_name)
      end
    end
  end
end

class Messages
  ...
  def post(payload, options={})
    ...
    with_cache_response(@queue.name, :messages) do |resp|
      resp = @client.parse_response(@client.post(path(options), to_send))
      # process single message post
      resp = {"id" => resp["ids"][0], "msg" => resp["msg"]} unless batch 
    end # returns ResponseBase
  end
end

This is simple example of caching system we may implement. We can use self-generated IDs for new queues. This solves that for now we could not get the info on non-existing queues from API. If such system is implemented client library will handle this itself by initializing info structure (size: 0, total_messages: 0, etc.) Cache must store queues' info data structures per name, not ID. This resolves issue with getting HTTP 404 on non-existing queues. Queue cache is initializes on first queue call and shares between all future requests thru the same IronMQ::Client object.

Also, to reflect API GET /projects/{Project ID}/queues/{Queue Name} I suggest to provide Queue#info method and turn [], Queue#id, etc. work thru info call.

What do you think, guys?

carimura commented 11 years ago

cc @edsrzf

featalion commented 11 years ago

It does make sense (at least for me) to unify this part in all client libraries, not only Ruby gem.

edsrzf commented 11 years ago

This seems fine to me. It doesn't look like anything in the API necessarily needs fixing, but let me know if I'm wrong or that changes.

featalion commented 11 years ago

@edsrzf ya, it does not needs any of API changes, you are right. But I suggested 2 ways how to do right logic. So, question mostly what we want there. Is it simple "call API each time any queue info requested" or "make cache layer and pass all API request thru".

treeder commented 11 years ago

I'd say drop the cache which solves most of this except the first one. Keep the reload method for backwards compatibility too.

For one: "Create new queue and call it for its size:", that method doesn't actually create a cache, it just references a queue, which may or may not exist. We could potentially add a create method: ironmq.queue("x").create to explicitly create it.

featalion commented 11 years ago

@treeder Got it. It seems easy and clear.

Will add the Queue#info method and update documentation. ironmq.queue("x") may create the queue if does not exists and return queue info. I think good for that is this POST request. Client library could call it on non-existing queue which creates new queue. It also requires no IronMQ backend changes. Seems more clear, I see no use case when developers can create queue objects but never use them. Again, we provide unlimited number of queues and automatic create seems better in this case for client libraries.

Does it makes sense?

treeder commented 11 years ago

I don't think it should create the queue when you call that method, that would require an extra api hit to check right? I think we should keep that explicit.

featalion commented 11 years ago

It does not for now. For now the gem hits /projects/{Project ID}/queues/{Queue Name} when Client#queue(name) is called. So, if we will need to create new queue it takes plus one API call, ofc.

treeder commented 11 years ago

I mean it shouldn't hit any endpoint when you just do ironmq.queue(name) should it?

featalion commented 11 years ago

Yes, this is possible but seems not IronMQ API reflection in this case. If you think we must reduce call numbers it has sense for me. And, ofc, cache is better to do it.

featalion commented 11 years ago

Done. IronMQ::Client#queue(name) does not call for queue info on IronMQ::Queue object initialization.