Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

migrating in_http to v0.14 API #1308

Merged
merged 11 commits into from
Nov 15, 2016
Merged

migrating in_http to v0.14 API #1308

merged 11 commits into from
Nov 15, 2016

Conversation

tagomoris
Copy link
Member

@tagomoris tagomoris commented Nov 9, 2016 •
edited
Loading

This change is to migrate in_http to v0.14 APIs, without socket/server plugin helpers (like #1306).

Currently, this change disables DetachMultiProcessMixin feature, because event_loop plugin helper can't work with it (error below occurs when DetachMultiProcessMixin is mixed in):

Assertion failed: (("libev: ev_loop recursion during release detected", loop_done != EVBREAK_RECURSE)), function ev_run, file ./../libev/ev.c, line 3437.
rake aborted!
SignalException: SIGABRT

@tagomoris tagomoris added the v0.14 label Nov 9, 2016
@tagomoris tagomoris force-pushed the migrate-http-plugin-to-v0.14-api branch from 9384c12 to fdb4aa8 Compare November 9, 2016 08:39
@tagomoris
migrating in_http to v0.14 API
1473619
@tagomoris
add #to_f: without this def, time.to_f is equal to time.to_int.to_f
31a559d
@tagomoris
convert regexp "format" into regexp parser explicitly
74be7bd 
Without this change, Plugin.new_parser will instantiate Fluent::TextParser instance.
It is a kind of Fluent::Compat::Parser, and will convert configuration for it (twice).
This double conversion breaks value modification of "types" parameter.
@tagomoris
check record is a Hash or not (nil? array?)
100e599
@tagomoris
fix to use NumericTimeParser to parse float time (without error of fl…
6a5490c 
…oating point value)
@tagomoris
fix wrong test codes
27be8bc
@tagomoris tagomoris force-pushed the migrate-http-plugin-to-v0.14-api branch from fdb4aa8 to 27be8bc Compare November 11, 2016 08:01
@tagomoris tagomoris changed the title [WIP] migrating in_http to v0.14 API migrating in_http to v0.14 API Nov 11, 2016
@tagomoris
Copy link
Member Author

@repeatedly Could you review this change?
How do you think about dropping support of detach_process?

@repeatedly
Copy link
Member

repeatedly commented Nov 11, 2016 •
edited
Loading

I will check it later.

How do you think about dropping support of detach_process?

I think this is no problem. detach_process itself doesn't boost the performance and current detach_process has several problems, e.g. internal pipe sometimes stuck.
And detach_process in in_http is un-documented feature: http://docs.fluentd.org/articles/in_http
So we can remove it. I will update v0.12 in_http to show warning message in configure.

@tagomoris
Fix to set time_type=string if "format" specifier is used for regular…
f87305c 
… expression directly.

It's because old TextParser estimates to parse "time" field without any time_format (using Ruby built-in formats).
@tagomoris
fix to set just expression as expression
6f5a2bf
repeatedly
repeatedly reviewed Nov 14, 2016
View reviewed changes
lib/fluent/plugin/in_http.rb
m = if @parser_configs.first['@type'] == 'in_http'
@parser_msgpack = parser_create(type: 'msgpack')
@parser_msgpack.estimate_current_event = false
@parser_json = parser_create(type: 'json')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parser_create uses usage for @_parsers key.
This second parser_create seems to overwrite msgpack parser.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, that's right.

repeatedly
repeatedly reviewed Nov 14, 2016
View reviewed changes
lib/fluent/plugin/in_http.rb
config_param :blocking_timeout, :time, default: 0.5
desc 'Set a white list of domains that can do CORS (Cross-Origin Resource Sharing)'
config_param :cors_allow_origins, :array, default: nil
desc 'Respond with empty gif image of 1x1 pixel.'
config_param :respond_with_empty_img, :bool, default: false

config_section :parse do
config_set_default :@type, 'in_http'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using nil bad idea?
It can avoid InHttpParser definition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, using nil is bad idea for here, because we can't specify nil explicitly in configuration file.

repeatedly
repeatedly reviewed Nov 14, 2016
View reviewed changes
test/plugin/test_in_http.rb
time_i = time.to_i

events = [
["tag1", time, {"REMOTE_ADDR"=>"129.78.138.66", "a"=>1}],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"REMOTE_ADDR"=>"129.78.138.66" is assigined by in_http, not emitted record.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'll fix it.

repeatedly
repeatedly reviewed Nov 14, 2016
View reviewed changes
test/plugin/test_in_http.rb
time_i = time.to_i
records = [{"a"=>1},{"a"=>2}]
events = [
["tag1", time, {"REMOTE_ADDR"=>"129.78.138.66", "a"=>1}],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Data to be committed is records in this test case, and events is used just for expectation.

@repeatedly
Copy link
Member

Commented. Others are looks good.

@tagomoris
Copy link
Member Author

I pushed two commits for review comments.

@repeatedly
Copy link
Member

LGTM

@tagomoris tagomoris merged commit 66158aa into master Nov 15, 2016
@tagomoris tagomoris deleted the migrate-http-plugin-to-v0.14-api branch November 15, 2016 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@repeatedly repeatedly repeatedly left review comments

Assignees

@repeatedly repeatedly

Labels
v0.14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tagomoris @repeatedly