MINIFICPP-706 - RawSiteToSite: remove code duplication#470
MINIFICPP-706 - RawSiteToSite: remove code duplication#470arpadboda wants to merge 1 commit intoapache:masterfrom
Conversation
arpadboda
force-pushed
the
MINIFICPP-706
branch
from
ca52408 to
b740654
Compare
|
Changes inherently look fine. Will run some tests before approval. Hope to send an email soon re a release so I'll wait for 0.7 to merge it. Thanks |
arpadboda
force-pushed
the
MINIFICPP-706
branch
2 times, most recently
from
21321d9 to
90640e7
Compare
| } | ||
|
|
||
| int RawSiteToSiteClient::readRespond(const std::shared_ptr<Transaction> &transaction, RespondCode &code, std::string &message) { | ||
| uint8_t firstByte; |
There was a problem hiding this comment.
This is again the case where the base class doesn't need to deal with the the innards of reading bytes. Probably means they can be removed from the base and made pure virtual since I believe they are overridden in http site to site if I'm not mistaken.
There was a problem hiding this comment.
That's the case.
The implementation here was similar to "ReadResponse", which exists in base class, where it's required.
The change I made was to call that instead of copy-pasting.
"ReadResponse" implementation however cannot be removed from base as http client relies on that: it overrides, but calls base implementation in a case.
There was a problem hiding this comment.
Then that reliance may not be valid any longer. I think some parent class details changed, but I will have to further investigate the HTTP portion. I want to fully understand this, instead of going off of memory. If it is a bug that can be a follow on, but once I take a closer look at HTTP it may become obvious. What the parent currently does, is an invalid reliance from the end of HTTP, but since things changed in some of the accessory code it may never get there, meaning we can make it pure virtual; however, I'll take a closer look at it while running to validate that it's not a bug before going that route.
There was a problem hiding this comment.
I'll also take another look at this PR closely and approve for merge in 0.7.0 if it doesn't change status quo for http ( which it doesn't look like it does at a quick glance ).
By the way, what scope of testing have you performed on this code via docker or in a deployed environment? I understand it's "moving code," but mistakes happen. Don't want to duplicate that effort.
There was a problem hiding this comment.
I didn't see a response for this. I have some time in the next few days to run tests. What have you been able to run? Thanks!
There was a problem hiding this comment.
Two things I did:
-Executed site2site related unit tests
-Verified transfers from MiNiFi to NiFi using the C examples (which still rely on C++ S2S implementation atm)
Any further verification is welcome and thanks in advance for that!
There was a problem hiding this comment.
Yep, I usually do full scope when anything touches core code, no matter how trivial the changes appear: docker tests, site to site secure, unsecure, transfers -- in this case I'll do raw and http just to be safe ( in minifi note C agent )...maybe more if I think it's prudent -- once all that is done I'll take another look at the PR and approve for merge in 0.7.0. Thanks.
|
@arpadboda can you force travis and appveyor to re-run on this so that we can do a little more eval and before merging this? |
arpadboda
force-pushed
the
MINIFICPP-706
branch
from
90640e7 to
5628ffd
Compare
Sure, rebased to master, pushed. |
This closes apache#470. Signed-off-by: Marc Parisi <phrocker@apache.org>
2 participants
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.