Counter API#1857
Conversation
* enable to use keyword argument at method arguments
|
@okkez If you want to test counter-api, use this branch instead. |
|
I've tried to test counter-api but it seems that counter-api is wok in progress. I will try to add some ad-hoc patch and test counter-api with Fluentd. |
| config_param :scope, :string | ||
|
|
||
| desc 'endpoint of counter server' | ||
| config_param :endpoint, :string # host:port |
There was a problem hiding this comment.
How about splitting host and port for consistency with other plugins such as in_forward and in_monitor_agent?
It is useful to set the default value for those parameters.
config_section :counter_server, multi: false do
config_param :bind, :string
config_param :port, :integer
end|
|
||
| config_section :counter_client, multi: false do | ||
| desc 'endpoint of counter client' | ||
| config_param :endpoint, :string # host:port |
There was a problem hiding this comment.
How about following? It is useful to set the default value for those parameters.
config_sectio :counter_client, multi: false do
config_param :host, :string
config_param :port, :integer
end| @port = opt[:port] || DEFAULT_PORT | ||
| @addr = opt[:addr] || DEFAULT_ADDR | ||
| @log = opt[:log] || $log | ||
| @conn = Connection.connect(@addr, @port, method(:on_message)) |
There was a problem hiding this comment.
How about following?
Fluent::Counter::Client is used by plugin developers and Counter API users. Is this right?
opt ={} does not describe details of parameters. Thus users have to read the code to understand this API.
We may also need plugin helper to create Counter API client for plugins.
def initialize(event_loop: Coolio::Loop.new, log: $log, host: '127.0.0.1', port: 4321)
@event_loop = event_loop
@host = host
@port = port
@log = log
@conn = Connection.connect(@host, @port, method(:on_message))
# ...
end|
|
||
| def after_run | ||
| stop_rpc_server if @rpc_endpoint | ||
| stop_counter_server if @counter_endpoint |
| exist_scope! | ||
| params = [params] unless params.is_a?(Array) | ||
| res = send_request('inc', @scope, params, options) | ||
| options[:async] ? res : res.get |
There was a problem hiding this comment.
How about like following?
def inc(params, options: {})
exist_scope!
params = [params] unless params.is_a?(Array)
res = send_request('inc', @scope, params, options)
if options[:async]
if block_given?
Thread.start do
yield res.get
end
else
res
end
else
if block_given?
yield res.get
else
res.get
end
end
endWe can use this as following:
@counter_client = ...
@counter_client.inc({ name: "counter", value: 1}, options: { async: true }) do |response|
# use response here
endIn the original code, we must handle future instance always when we send request asynchronously.
| def get | ||
| # Block until `set` method is called and @result is set a value | ||
| join if @result.nil? | ||
| @result |
There was a problem hiding this comment.
How about changing the return value to Response object from plain Hash?
class Response
attr_reader :errors, :data
def initialize(result)
@errors = result["errors"]
@data = result["data"]
# ...
end
def success?
@errors.nil? || @errors.empty?
end
def error?
!success?
end
endIt is useful to check Counter API response as following:
@counter_client.inc({ name: "counter", value: 1}, options: { async: true }) do |response|
if response.success?
log.debug("Update counter successfully")
else
log.warn("Counter API error: #{response.errors}")
end
endOriginal code:
future = @counter_client.inc({ name: "counter", value: 1}, options: { async: true })
Thread.start do
response = future.get
if future.errors?
log.error("failure")
else
log.debug("success")
end
endThere was a problem hiding this comment.
Looks good. I will apply the changes soon.
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
|
@okkez Do you have other suggestion? If not, I have a plan to merge this PR. |
|
@repeatedly I want a shorthand for How about that the counter client sends an asynchronous request by default? Or, how about that plugin helper creates an asynchronous client? async_client = counter_client_create(scope: "datacounter", async: true)
async_client.inc(name: "name", value: 1) # send an asynchronous request
async_client.inc(name: "name", value: 1, options: { async: false }) # send a synchronous requestOr else...? |
I see. One idea is removing sync API. Adding How about this? |
|
Looks good to me 👍 |
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
|
Remove sync API |
|
Merged. We will improve Counter API with actual plugins. |
Update version of #1241
We continue to update a patch here.