Add string based time duration support.

[guyboertje]

Fixes #187

[colinsurprenant]

not sure I understand the reason for returning a string to represent an error condition. why not just raise an exception? I think it will make the error handling code much easier to read and follow?

[guyboertje]

I just followed the pattern from validate_value in the config dsl mixin.
I will improve it.

[colinsurprenant]

This is neat. I am wondering what the plan would be to make this generic and usable from other plugins?

[guyboertje]

@colinsurprenant
It is neat. The Slow logger settings validations has something similar but you can't set a "units" hint to retain BWC when an number means a non-seconds value and the coerced number is in nanoseconds.

As to making it reusable, I would like to do so but it depends on what the core team do with the Java Plugin API WRT custom validation and having an interface to implement for a such a class.

[colinsurprenant]

@guyboertje

As to making it reusable, I would like to do so but it depends on what the core team do with the Java Plugin API WRT custom validation and having an interface to implement for a such a class.

Or the generic part of could be extracted as a gem, and then it could be reused elsewhere too. I'll let you followup on this if you feel this could bring value.

So the only conceptual problem I see is that this relies on swapping implementation details and could possibly break upon upgrading core. I wonder if we could avoid this here?

[guyboertje]

So there are three parts to the mechanism (same with the custom validators in jdbc_static).

1. the alias_method and alternative method patch
2. the validator class(es)
3. the 'special' return type indicating pass/fail

1 Is definitely susceptible to a change in core.
2 Is immune but should probably be a jruby extension.
3 Might be immune in that it is a simple Struct. The receiver can use it as desired.

IMO, 1 need to be in the Plugin API with a validation hooks style registry.

[colinsurprenant]

@guyboertje WDYT in opening a followup issue for this - I think we should keep tab on this and have some kind of strategy to prevent future regressions because no one realized that some plugins were depending on the validation implementation.

[guyboertje]

@colinsurprenant
elastic/logstash#9733

[colinsurprenant]

LGTM

[fbaligand]

Hi @guyboertje

Thanks for this great enhancement !

Given that for some options, types and default values have changed, it would be great to also update documentation, I mean : docs/index.asciidoc

[fbaligand]

Sorry for my remark : I just realized it is already done in the following PR.