Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upCounter API #1857
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. |
|
I've noticed some points. |
| config_param :scope, :string | ||
|
|
||
| desc 'endpoint of counter server' | ||
| config_param :endpoint, :string # host:port |
okkez
Mar 30, 2018
Contributor
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
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 |
okkez
Mar 30, 2018
Contributor
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
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)) |
okkez
Mar 30, 2018
Contributor
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
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| socket_manager_path = ServerEngine::SocketManager::Server.generate_path | ||
| ServerEngine::SocketManager::Server.open(socket_manager_path) | ||
| ENV['SERVERENGINE_SOCKETMANAGER_PATH'] = socket_manager_path.to_s | ||
| 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 |
okkez
Apr 2, 2018
Contributor
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
end
We can use this as following:
@counter_client = ...
@counter_client.inc({ name: "counter", value: 1}, options: { async: true }) do |response|
# use response here
end
In the original code, we must handle future instance always when we send request asynchronously.
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.
repeatedly
Apr 5, 2018
Author
Member
Added suggested API.
Added suggested API.
| def get | ||
| # Block until `set` method is called and @result is set a value | ||
| join if @result.nil? | ||
| @result |
okkez
Apr 2, 2018
Contributor
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
end
It 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
end
Original 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
end
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
end
repeatedly
Apr 4, 2018
Author
Member
Looks good. I will apply the changes soon.
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.