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.

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

config: Add hostname and worker_id short-cut. fix #1473 #1814

Merged
merged 2 commits into from
Jan 16, 2018

Conversation

repeatedly
Copy link
Member

No description provided.

@repeatedly repeatedly requested a review from mururu January 10, 2018 10:59
@repeatedly repeatedly self-assigned this Jan 10, 2018
@repeatedly repeatedly added v1 enhancement Feature request or improve operations labels Jan 10, 2018
@@ -162,6 +163,8 @@ def eval_embedded_code(code)
if @eval_context.nil?
parse_error! "embedded code is not allowed in this file"
end
hostname = Socket.gethostname
worker_id = ENV['SERVERENGINE_WORKER_ID'] || ''
Copy link
Member Author

Choose a reason for hiding this comment

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

This approach shows warnings with verbose mode but I'm not sure how to avoid it...

Copy link
Member

Choose a reason for hiding this comment

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

I can't see any warnings on my environment. Could you share the detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. How about using Binding#local_variable_set?

Copy link
Member Author

Choose a reason for hiding this comment

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

With instance_eval, local_variable_set doesn't work. Try this:

context = Kernel.binding
context.local_variable_set(:worker_id, 1)
p context.instance_eval('"foo#{worker_id}"')

Copy link
Member

Choose a reason for hiding this comment

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

Can't we use #eval instead of #instance_eval here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it doesn't break existing behaviour or not.

@nurse Do you see any problem for using eval here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just add to code seems simple.

code = <<eom + code
hostname = Socket.gethostname
worker_id = ENV['SERVERENGINE_WORKER_ID'] || ''
eom
         @eval_context.instance_eval(code)

@mururu
Copy link
Member

mururu commented Jan 10, 2018

With this conf

<source>
  @type dummy
  @id "dummy#{worker_id}"
  tag test
</source>
<match test>
  @type null
</match>

dumped config is like this:

2018-01-11 01:04:34 +0900 [info]: using configuration file: <ROOT>
  <source>
    @type dummy
    @id dummy
    tag "test"
  </source>
  <match test>
    @type null
  </match>
</ROOT>

but I think @id "dummy#{worker_id}" is better.

@repeatedly
Copy link
Member Author

Yeah. The problem is "#{}" is processed during configuration parsing and it doesn't store raw parameter.
This is the Element problem so we need other issue/pr for it.

@mururu
Copy link
Member

mururu commented Jan 11, 2018

Makes sense.

@repeatedly
Copy link
Member Author

Applied reviews. Will merge after test passed.

@repeatedly repeatedly merged commit d210c68 into master Jan 16, 2018
@repeatedly repeatedly deleted the add-hostname-and-worker_id-short-cut branch January 16, 2018 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or improve operations v1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants