Bucket name from placeholder #363
Conversation
rmontenegroo
force-pushed
the
bucket-name-from-placeholder
branch
2 times, most recently
from
0fd013d to
701911b
Compare
lib/fluent/plugin/out_s3.rb
Outdated
| bucket_name = nil | ||
|
|
||
| if @s3_bucket =~ /\$\{.*\}/ | ||
| @s3_bucket.scan(/\$\{([^\$\{\}]+)\}/) do |placeholder| |
There was a problem hiding this comment.
Time placeholders does not use $ character.
We can use Validator class instead.
https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/output.rb#L607
There was a problem hiding this comment.
if !@tag_placeholders.empty? || !@keys_placeholders.empty? || !@time_placeholders.empty??
There was a problem hiding this comment.
Tag, time and keys placeholders are scanned on @s3_bucket on star. If they're empty, the bucket is created during the star and anytime else in the future.
On the other way, if placeholders are found in @s3_bucket, it means bucket object must be evaluated, everything time before a buffer flush. This is done inside the write method. In the write method I check this situation evaluating whether @bucket is instantiated or not.
|
@cosmo0920 why get_placeholders_time does not return any time placeholder if I set @s3_bucket to "test-%Y" for example? |
|
The thing is PlaceholderValidator does not allow a timekey larger than a day (https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/output.rb#L686) so it won't be possible to validate time placeholders for year and month using the validator. Am I right? Something like "test-%Y-%m" won't be accepted by validator as placeholders. |
rmontenegroo
force-pushed
the
bucket-name-from-placeholder
branch
from
608e703 to
c69893c
Compare
|
@cosmo0920 I implemented a simple ext_get_placeholders_time only to check if there are time markers (%Y %m %d %H %M %S) in a string. With this I am able to check time placeholders, and with Validator class I can check tag and keys placeholders. I hope it is satisfying. |
Right. |
| @@ -427,7 +427,7 @@ def setup_mocks(exists_return = false) | |||
| mock(Aws::S3::Client).new(anything).at_least(0) { @s3_client } | |||
| @s3_resource = mock(Aws::S3::Resource.new(client: @s3_client)) | |||
| mock(Aws::S3::Resource).new(client: @s3_client) { @s3_resource } | |||
| @s3_bucket = mock(Aws::S3::Bucket.new(name: "test", | |||
| @s3_bucket = mock(Aws::S3::Bucket.new(name: "test_bucket", | |||
There was a problem hiding this comment.
Why this change is needed?
To get merged this PR, we should add placeholders testcases.
There was a problem hiding this comment.
I sincerely don't know. I thought someone changed it last patch. It was producing an error on tests I guess. I will check it later.
Yeah... I was afraid you would say that about testing placeholders. I truly have no idea how to do it right now since they're evaluated during plug-in execution time.
Any suggestions? If someone give a hint I might get through it. I am no expert on ruby tests though.
There was a problem hiding this comment.
Yeah... I was afraid you would say that about testing placeholders. I truly have no idea how to do it right now since they're evaluated during plug-in execution time.
These testcases are fluent-plugin-elasticsearch's but they shows how to write placeholders testcases:
https://github.com/uken/fluent-plugin-elasticsearch/blob/c0a4713edfc620cf566ede02f0f85930d1c5a88a/test/plugin/test_out_elasticsearch.rb#L3893
- Write configuration which includes
<buffer attributes1, attributes2, ..., attributesN>withFluent::Config::Element.newand using tag/time/user defined(variables) placeholders - Creates chunk metadata keys which are originated from
<buffer attributes1, attributes2, ..., attributesN>(This is automatically created when use test driver#feed) - Assert placeholders replaced values with test-unit assertions
There was a problem hiding this comment.
We use !, &&, and || instead of not, and, and or operator.
This is because symbol versions are higher precedence than not, and, and or operators. This is some pitfall of Ruby.
https://ruby-doc.org/core-2.4.0/doc/syntax/precedence_rdoc.html
Signed-off-by: rmontenegroo <[email protected]>
rmontenegroo
force-pushed
the
bucket-name-from-placeholder
branch
from
c69893c to
b5151b4
Compare
|
This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days |
|
This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days |
|
This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days |
This patch allows the use of placeholders to setup bucket names.
There is a new parameter s3_bucket_fallback to use in cases when at least one placeholder is not found amoung chunk keys.