You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/05/15 14:13:46 UTC
[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
adamdebreceni opened a new pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786
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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r426403568
##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -206,25 +207,36 @@ class FlowFileResponder : public CivetHandler {
minifi::io::CivetStream civet_stream(conn);
minifi::io::CRCStream < minifi::io::CivetStream > stream(&civet_stream);
uint32_t num_attributes;
+ int read;
uint64_t total_size = 0;
- total_size += stream.read(num_attributes);
+ read = stream.read(num_attributes);
+ if(!isServerRunning())return false;
Review comment:
apparently they do not segfault, but simply indicate error in their return value [mg_read](https://github.com/civetweb/civetweb/blob/master/docs/api/mg_read.md)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r426653075
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -145,17 +145,15 @@ class CRCStream : public BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename K>
- std::vector<uint8_t> readBuffer(const K& t) {
- std::vector<uint8_t> buf;
+ int readBuffer(std::vector<uint8_t>& buf, const K& t) {
Review comment:
This leaves the option of a new overload with proper error handling. If existing 3rd party extensions happened to use the old overload, it should work similarly, even if without error checking.
If you have a better idea, feel free to propose it. I have a few more as well, but they also highly deviate from the style of the codebase with limited benefit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#issuecomment-629391194
Looking at CI logs, there is something left:
https://travis-ci.org/github/apache/nifi-minifi-cpp/jobs/687464775
The last debug line before the failure of the testcase is pretty talkative:
```
[2020-05-15 14:46:30.368] [org::apache::nifi::minifi::processors::ListenHTTP::Handler] [debug] ListenHTTP handling POST request of length -1
```
ListenHTTP also uses civetweb to achieve listening, so we might have the same issue there as we have with tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r426404826
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -145,17 +145,15 @@ class CRCStream : public BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename K>
- std::vector<uint8_t> readBuffer(const K& t) {
- std::vector<uint8_t> buf;
+ int readBuffer(std::vector<uint8_t>& buf, const K& t) {
Review comment:
the problem is that every other read*/write* method confirms to the "return the number of read/wrote bytes or a negative number on error", throwing would deviate from that significantly, do we have a list of dependant projects, or a guide on what we consider part of the api?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r425895222
##########
File path: extensions/http-curl/client/HTTPStream.h
##########
@@ -155,13 +155,13 @@ class HttpStream : public io::BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename T>
- std::vector<uint8_t> readBuffer(const T&);
+ int readBuffer(std::vector<uint8_t>&, const T&);
Review comment:
Correct, it's an inline non-virtual member function that happens to be defined in many stream classes the same way. A lot of code duplication, but works.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda commented on pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#issuecomment-630924894
CI jobs green, executed the tests parallel on my server 3 times, still all green. Nice work!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r425865244
##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -206,25 +207,36 @@ class FlowFileResponder : public CivetHandler {
minifi::io::CivetStream civet_stream(conn);
minifi::io::CRCStream < minifi::io::CivetStream > stream(&civet_stream);
uint32_t num_attributes;
+ int read;
uint64_t total_size = 0;
- total_size += stream.read(num_attributes);
+ read = stream.read(num_attributes);
+ if(!isServerRunning())return false;
Review comment:
Why?
I would expect not even trying to read the stream when the server is not running.
Edit: okay, I get it, we check at every stream access.
Wonder if stream reads in this case might cause segfaults as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r425865244
##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -206,25 +207,36 @@ class FlowFileResponder : public CivetHandler {
minifi::io::CivetStream civet_stream(conn);
minifi::io::CRCStream < minifi::io::CivetStream > stream(&civet_stream);
uint32_t num_attributes;
+ int read;
uint64_t total_size = 0;
- total_size += stream.read(num_attributes);
+ read = stream.read(num_attributes);
+ if(!isServerRunning())return false;
Review comment:
Why?
I would expect not even trying to read the stream when the server is not running.
##########
File path: extensions/http-curl/client/HTTPStream.h
##########
@@ -155,13 +155,13 @@ class HttpStream : public io::BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename T>
- std::vector<uint8_t> readBuffer(const T&);
+ int readBuffer(std::vector<uint8_t>&, const T&);
Review comment:
I didn't check the code of BaseStream, just to confirm: the original function was declared at this level and it's not an override of a base func?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r426648842
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -145,17 +145,15 @@ class CRCStream : public BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename K>
- std::vector<uint8_t> readBuffer(const K& t) {
- std::vector<uint8_t> buf;
+ int readBuffer(std::vector<uint8_t>& buf, const K& t) {
Review comment:
In theory everything in libminifi headers is API. In practice we sometimes break non-core APIs if it fixes important issues or only breaks arcane uses. IIRC the stream APIs are considered "core" (important for 3rd party extension development), but @arpadboda will be able to confirm/correct me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r425851554
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -145,17 +145,15 @@ class CRCStream : public BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename K>
- std::vector<uint8_t> readBuffer(const K& t) {
- std::vector<uint8_t> buf;
+ int readBuffer(std::vector<uint8_t>& buf, const K& t) {
Review comment:
While I agree with your change, this breaks libminifi API. We could consider providing an overload with error handling or modify the original to throw in case of an error, instead of returning an error code.
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -138,10 +138,10 @@ class VerifyRWTimeoutInvokeHTTP : public VerifyInvokeHTTP {
};
void run(VerifyInvokeHTTP& harness,
- const std::string& url,
- const std::string& test_file_location,
- const std::string& key_dir,
- CivetHandler * handler) {
+ const std::string& url,
+ const std::string& test_file_location,
+ const std::string& key_dir,
+ ServerAwareHandler * handler) {
Review comment:
The previous continuation indentation of 2 levels (i.e. 4 spaces) was correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r426653075
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -145,17 +145,15 @@ class CRCStream : public BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename K>
- std::vector<uint8_t> readBuffer(const K& t) {
- std::vector<uint8_t> buf;
+ int readBuffer(std::vector<uint8_t>& buf, const K& t) {
Review comment:
This leaves the option of a new overload with proper error handling. If existing 3rd party extensions happened to use the old overload, it should work similarly, even is without error checking.
If you have a better idea, feel free to propose it. I have a few more as well, but they also highly deviate from the style of the codebase with limited benefit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda edited a comment on pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda edited a comment on pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#issuecomment-629391194
Looking at CI logs, there is something left:
https://travis-ci.org/github/apache/nifi-minifi-cpp/jobs/687464775
The last debug line before the failure of the testcase is pretty talkative:
```
[2020-05-15 14:46:30.368] [org::apache::nifi::minifi::processors::ListenHTTP::Handler] [debug] ListenHTTP handling POST request of length -1
```
ListenHTTP also uses civetweb to achieve listening, so we might have the same issue there as we have with tests.
The nice way there would be to stop handling any request after notifyStop was called.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r425865244
##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -206,25 +207,36 @@ class FlowFileResponder : public CivetHandler {
minifi::io::CivetStream civet_stream(conn);
minifi::io::CRCStream < minifi::io::CivetStream > stream(&civet_stream);
uint32_t num_attributes;
+ int read;
uint64_t total_size = 0;
- total_size += stream.read(num_attributes);
+ read = stream.read(num_attributes);
+ if(!isServerRunning())return false;
Review comment:
Why?
I would expect not even trying to read the stream when the server is not running.
Edit: okay, I get it, we check at every stream access.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r427085519
##########
File path: libminifi/include/io/CRCStream.h
##########
@@ -145,17 +145,15 @@ class CRCStream : public BaseStream {
protected:
/**
- * Creates a vector and returns the vector using the provided
- * type name.
+ * Populates the vector using the provided type name.
+ * @param buf output buffer
* @param t incoming object
- * @returns vector.
+ * @returns number of bytes read.
*/
template<typename K>
- std::vector<uint8_t> readBuffer(const K& t) {
- std::vector<uint8_t> buf;
+ int readBuffer(std::vector<uint8_t>& buf, const K& t) {
Review comment:
readded the previous versions to libminifi
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #786: MINIFICPP-1225 - Fix flaky HTTP tests.
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #786:
URL: https://github.com/apache/nifi-minifi-cpp/pull/786#discussion_r427205135
##########
File path: extensions/civetweb/processors/ListenHTTP.h
##########
@@ -201,6 +201,8 @@ class ListenHTTP : public core::Processor {
}
return 0;
}
+ protected:
+ void notifyStop() override;
Review comment:
(Correctly) adding `override` to this virtual function triggered clang's `-Winconsistent-missing-override` warnings for all other member functions that override virtual functions. Could you add `override` to at least `onTrigger`, `initialize` and `onSchedule` as well (but preferably to all such functions in the file)?
https://travis-ci.org/github/apache/nifi-minifi-cpp/jobs/688702969#L12034
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override
https://google.github.io/styleguide/cppguide.html#Inheritance
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org