Skip to content

Minificpp-780 - Change GenerateFlowFile to allow 0b content FlowFiles#636

Closed
arpadboda wants to merge 1 commit intoapache:masterfrom
arpadboda:MINIFICPP-780
Closed

Minificpp-780 - Change GenerateFlowFile to allow 0b content FlowFiles#636
arpadboda wants to merge 1 commit intoapache:masterfrom
arpadboda:MINIFICPP-780

Conversation

@arpadboda
Copy link
Contributor

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:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

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.

@arpadboda arpadboda changed the title Minificpp 780 Minificpp-780 - Change GenerateFlowFile to allow 0b content FlowFiles Aug 29, 2019
@arpadboda
Copy link
Contributor Author

WARNING: this PR depends on #637 - that one should be merged first, otherwise tests included in this are going to fail.

@arpadboda
Copy link
Contributor Author

arpadboda commented Aug 29, 2019

While checking this processor I found the following issues:
-In case batch count is greater than one, the flowfiles generated in one batch are not unique
-Handling of 0B size was error-prone: allocating zero size arrays, passing null pointers to callbacks, etc
-A potential memory segmentation violation in case the file size is not multiple of 4.

Because of these it was easier to completely rewrite the logic than fixing it.

@arpadboda arpadboda force-pushed the MINIFICPP-780 branch 4 times, most recently from 8a1c9cf to 83d12e7 Compare September 4, 2019 14:26
@arpadboda
Copy link
Contributor Author

@phrocker : comments addressed here, could you revisit?

Copy link
Contributor

@bakaid bakaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, a few issues in inline comments.

The GenerateFlowFileTestEmpty test fails because PutFile can't put 0-sized file. Please rebase to a fresh master where that is fixed, I'll reverify it.

std::random_device rd;
std::mt19937 eng(rd());
if (textData) {
std::uniform_int_distribution<> distr(0, TEXT_LEN);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is legacy, but it took me a while to figure out why this is right. The TEXT_CHARS array contains 91 elements, so TEXT_LEN in reality is not the length of the array, but the maximum index. I think the previous version, which used % TEXT_LEN was erroneus, and now that we generate indexes from [0, TEXT_LEN] we are good, but it is not easy to understand.
I'm OK with merging this, but it should be replaced with something more straightforward eventually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

integer module should be 0 to n-1 inclusive, so erroneous because it's a max index and not a length? I don't necessarily care what the hold code did but this block does need some comments to clarify the change why it is how it is and if the names are misleading a clarification to the reason why it was wrong previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the array with a C string, so we don't need the length any more.

Copy link
Contributor

@phrocker phrocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor things but it seems that there may be some comments that can help clarify the code to hep maintain it.

Copy link
Contributor

@bakaid bakaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tests also run successfully now.

@arpadboda arpadboda closed this in aa65f5d Nov 10, 2019
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