MINIFICPP-681 - Add content hash processor #445
Conversation
arpadboda
force-pushed
the
MINIFICPP-681
branch
3 times, most recently
from
4bc64af
to
e58aac6
Compare
|
taking a look. |
| ret_val.first = digestToString(digest, SHA_DIGEST_LENGTH); | ||
| return ret_val; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can these functions be combined to reduce duplication?
There was a problem hiding this comment.
Honestly I wanted to do but failed, given the digest, the init and the fine calls, moreover the digest length const all differ.
The header file is pure C, some tempalte magic there could easily help to reduce code duplication here, but I don't think we should do that and add it here. Maybe to a util file, but definitely not to a processor.
| // Erase '-' to make sha-256 and sha-2 work, too | ||
| algoName.erase(std::remove(algoName.begin(), algoName.end(), '-'), algoName.end()); | ||
|
|
||
| // This throws in case algo is not found, but that's fine |
There was a problem hiding this comment.
Might the code above be less duplicative with a simple if statement here?
| // Erase '-' to make sha-256 and sha-2 work, too | ||
| algoName.erase(std::remove(algoName.begin(), algoName.end(), '-'), algoName.end()); | ||
|
|
||
| // This throws in case algo is not found, but that's fine |
There was a problem hiding this comment.
Curious about the comment, "This throws in case algo is not found, but that's fine" What do you mean by "that's fine?"
That would cause a rollback, which may then put back pressure on the flow. This may not be desired. It doesn't allow the user to gracefully deal with the failure relationship. Might there be a way to deal with this such that failure is a condition we can account for in our relationships?
There was a problem hiding this comment.
This is definitely a interesting point.
I would consider failure relationship being used for flowfile-specific and environment issues mostly. For eg. the processor expects some data to be present in attributes/content of the flowfile, and this criteria isn't met, putfile cannot write as disk is full, getfile has no permission to read, etc.
Although this case is about the processor being misconfigured, which I consider to be a bit different case. In an ideal world this shoudn't even happen as setting the property should fail in case the provided value is not one of the allowed ones.
There was a problem hiding this comment.
Failure is a generic term and is generally up to the independent processor to define -- your definition is not true across all processors in NiFi, so I'm fine leaving that up to the author to decide in this case; however, if the empty content case accounted for within the processor? Some have perceived empty content hashing as a potential failure case. Can make that a follow on task, though, but we should probably reach parity with NiFI eventually
|
|
||
| const auto& ret_val = algo(stream); | ||
|
|
||
| if (ret_val.second <= 0) { |
There was a problem hiding this comment.
Is this necessary? Cryptographic hash functions ensure the result will never be empty. If the stream failed the digest string would be empty. This would make the return code unnecessary.
There was a problem hiding this comment.
Rerturn code was originally introduced to meet the requirements of readCallback: it's int64_t return value expexts the number of bytes read to be returned.
The condition here can be removed, we can stamp the empty hash as well.
| ret = stream->readData(buffer, HASH_BUFFER_SIZE); | ||
| if(ret > 0) { | ||
| MD5_Update(&context, buffer, ret); | ||
| ret_val.second += ret; |
There was a problem hiding this comment.
As mentioned, below, the HashReturnType seems like it might not be necessary. If ret < 0 on any given rad you exit the conditional and loop, then you proceed to call finalize on the hash functions with that partially written context. The code then supplies a digest that is potentially incorrect. Alternatively you can simply short circuit and return an empty string on any stream error and be guaranteed that the resulting hash is an error case.
There was a problem hiding this comment.
As per the previous comment, ret is to used as return value of the readCallback.
arpadboda
force-pushed
the
MINIFICPP-681
branch
from
e58aac6
to
c4f02d4
Compare
arpadboda
force-pushed
the
MINIFICPP-681
branch
from
c4f02d4
to
c2c8983
Compare
| std::string value; | ||
|
|
||
| attrKey_ = (context->getProperty(HashAttribute.getName(), value)) ? value : "Checksum"; | ||
| algoName_ = (context->getProperty(HashAlgorithm.getName(), value)) ? value : "MD5"; |
There was a problem hiding this comment.
Default should probably be sha-256. I believe NiFi has transitioned to this.
There was a problem hiding this comment.
I'm not sure that's "low power friendly," but they can deal with that through configuration.
|
You can also look at |
This closes apache#445. Signed-off-by: Marc Parisi <phrocker@apache.org>
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.