Adding HashField transformer #12
Conversation
…ed hashing algorithm to transform the targeted value. ISSUE-9.
brbrown25
force-pushed
the
feature/ISSUE-9
branch
from
2a81075 to
d8aa4cb
Compare
|
@brbrown25 thank you 👍 |
|
|
||
| static final String hashString(final String hashAlg, final String input) { | ||
| try { | ||
| final MessageDigest md = MessageDigest.getInstance(hashAlg); |
There was a problem hiding this comment.
Could you please initialize this somewhere in configure? MessageDigest.getInstance might be costly enough to not want to call it on each message.
There was a problem hiding this comment.
Yeah that’s a good call out I’ll make that change.
- initialize in the configure to be more efficient
|
Thanks for your contribution @brbrown25. I'll go through it soon |
|
Thanks @ivanyu I based it off the topic replacement transformer for consistency and can tweak as needed. Thanks for the opportunity! |
| .define( | ||
| FUNCTION_CONFIG, | ||
| ConfigDef.Type.STRING, | ||
| null, |
There was a problem hiding this comment.
I think it should be ConfigDef.NO_DEFAULT_VALUE here, since we don't want to have null as the function name
There was a problem hiding this comment.
- switch to
ConfigDef.NO_DEFAULT_VALUE
| return Optional.of(rawFieldName); | ||
| } | ||
|
|
||
| Optional<String> hashFunction() { |
There was a problem hiding this comment.
The empty string is excluded by the validator, null we can also exclude. Thus, Optional won't be needed.
And since the set of possible values is limited, HashFunction could be returned from here.
src/main/java/io/aiven/kafka/connect/transforms/HashFieldConfig.java
Outdated
Show resolved
Hide resolved
|
Would you consider adding |
Yeah I wasn't sure if that was a desired setting or not, I think it makes sense to add it.
|
|
@ivanyu addressed all of the pr feedback, let me know what other changes you'd like and collapse this into a single commit. |
ivanyu
changed the title
|
Merging, thanks @brbrown25 |
|
@brbrown25 @ivanyu thank you both very much 👍 |
Addresses #9