Skip to content

MINIFI-190: Creating initial commit with additional tests#47

Closed
phrocker wants to merge 1 commit intoapache:masterfrom
phrocker:MINIFI-190
Closed

MINIFI-190: Creating initial commit with additional tests#47
phrocker wants to merge 1 commit intoapache:masterfrom
phrocker:MINIFI-190

Conversation

@phrocker
Copy link
Contributor

@phrocker phrocker commented Feb 9, 2017

Additional tests will be updated in separate tickets as needed.

Resolves MINIFI-194 by using a semaphore in place of the
FlowController instance. Stop is performed outside of the
signal handler to avoid synchronicity issues.

Resolves MINIFI-192 by using lock_guard based on a conditional

Resolves issues found with MINIFI-190 regarding GetFile. Added
pragma definitions for GCC < 4.9

@phrocker
Copy link
Contributor Author

phrocker commented Feb 9, 2017

I'll be happy to break this apart

@phrocker
Copy link
Contributor Author

phrocker commented Feb 9, 2017

I have found that the flow repository isn't cleaning up after itself. Will resolve that first.

@phrocker phrocker force-pushed the MINIFI-190 branch 4 times, most recently from 86ac591 to 2e7fadb Compare February 10, 2017 01:43
@apiri
Copy link
Member

apiri commented Feb 10, 2017

Hey @phrocker,

Are you still iterating on this particular PR or is it ready for review?

Resolves MINIFI-194 by using a semaphore in place of the
FlowController instance. Stop is performed outside of the
signal handler to avoid synchronicity issues.

Resolves MINIFI-192 by using lock_guard based on a conditional

Resolves issues found with MINIFI-190 regarding GetFile. Added
pragma definitions for GCC < 4.9
@phrocker
Copy link
Contributor Author

@apiri Yes. I just noticed a typo, but it should be ready. It'll probably have some adjustments along the review pipeline, but I don't plan anything else in this PR unless there are comments or things such as typos.

@phrocker
Copy link
Contributor Author

@apiri I did not add the test I have for DataStream and Serializable in this PR, since I'm using it for another PR I'm working on -- this was simply a function of breaking apart PRs. I'll be happy to port those to this PR. I don't typically submit PRs without tests, but also wanted to break the PRs apart. Please advise. It won't be a great deal of work to bring those tests into this branch.

@apiri
Copy link
Member

apiri commented Feb 10, 2017

@phrocker Flexible either way. Just wanted to be sure things were getting moved through and incorporated. Let me know when ready and can scope out then. Thanks!

@phrocker
Copy link
Contributor Author

@apiri Okay, cool. I think reviewing these changes then allow the subsequent changes to be in a separate PR ( it's coming ) is probably better since those tests encompass other parts of the code too. Plus getting this through sooner with any changes as a result of comments is probably more advantageous.

@apiri
Copy link
Member

apiri commented Feb 10, 2017

@phrocker Sounds good. Will scope these out a bit later or over the weekend.

@apiri
Copy link
Member

apiri commented Feb 12, 2017

reviewing

@apiri
Copy link
Member

apiri commented Feb 12, 2017

Changes look good and was able to verify build and functionality on several environments. There were a few errant new lines (one of which was in the license header) which I noticed that I will clean up on merge.

@asfgit asfgit closed this in 09d973b Feb 12, 2017
nghiaxlee pushed a commit to nghiaxlee/nifi-minifi-cpp that referenced this pull request Jul 8, 2019
Resolves MINIFI-194 by using a semaphore in place of the
FlowController instance. Stop is performed outside of the
signal handler to avoid synchronicity issues.

Resolves MINIFI-192 by using lock_guard based on a conditional

Resolves issues found with MINIFI-190 regarding GetFile. Added
pragma definitions for GCC < 4.9

This closes apache#47.

Signed-off-by: Aldrin Piri <aldrin@apache.org>
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.

2 participants