Skip to content

KAFKA-3237 - Remove test cases testInvalidDefaultRange() and testInva…#936

Closed
jcustenborder wants to merge 1 commit into
apache:trunkfrom
jcustenborder:KAFKA-3237
Closed

KAFKA-3237 - Remove test cases testInvalidDefaultRange() and testInva…#936
jcustenborder wants to merge 1 commit into
apache:trunkfrom
jcustenborder:KAFKA-3237

Conversation

@jcustenborder
Copy link
Copy Markdown
Contributor

Remove test cases testInvalidDefaultRange() and testInvalidDefaultString(). Defaults if not overridden will get checked on parse. Testing the defaults is unnecessary. This allows you to set that a parameter is required while setting a validator for that parameter. Added a test case testNullDefaultWithValidator that allows a null default with a validator for certain strings.

@granthenke
Copy link
Copy Markdown
Member

If the goal is to have a required parameter with a validator, an alternative solution would be to add a new method to ConfigDef that accepts a validator with no default. The problem is it collides with the existing signature that accepts a default Object with no validator.

public ConfigDef define(String name, Type type, Object defaultValue, Importance importance, String documentation)
...
public ConfigDef define(String name, Type type, Validator validator, Importance importance, String documentation)

Perhaps a fix utilizing ConfigDef.NO_DEFAULT_VALUE could work. It would need to be made public. Then the validator check that was removed could be changed to:

if (this.validator != null && this.hasDefault())
  this.validator.ensureValid(name, defaultValue);

The reason I am not a fan of the existing change is that null really isn't a valid default, and if a default is defined, it is assumed that the parameter is not required and whatever is set should be valid in the provided validator. The provided default of null will also be printed in the generated docs output and sorted as if it not required. Example: http://kafka.apache.org/documentation.html#producerconfigs

@jcustenborder
Copy link
Copy Markdown
Contributor Author

@granthenke I didn't think about the documentation generation use case. I'll add the correct this and resubmit.

… public and added check for hasDefault(). Original tests work and new test testNullDefaultWithValidator() does as well.
@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Mar 9, 2016

LGTM

@asfgit asfgit closed this in 090d722 Mar 9, 2016
efeg added a commit to efeg/kafka that referenced this pull request Jan 29, 2020
wcarlson5 pushed a commit to wcarlson5/kafka that referenced this pull request Feb 6, 2024
upgrading netty to 4.1.96.Final
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants