arpadboda commented on a change in pull request #775:
URL: https://github.com/apache/nifi-minifi-cpp/pull/775#discussion_r419326908



##########
File path: extensions/http-curl/client/HTTPClient.cpp
##########
@@ -296,12 +296,13 @@ bool HTTPClient::submit() {
   }
   curl_easy_setopt(http_session_, CURLOPT_HEADERFUNCTION, 
&utils::HTTPHeaderResponse::receive_headers);
   curl_easy_setopt(http_session_, CURLOPT_HEADERDATA, 
static_cast<void*>(&header_response_));
-  if (keep_alive_probe_ > 0){
-    logger_->log_debug("Setting keep alive to %d",keep_alive_probe_);
+  if (keep_alive_probe_.count() > 0) {
+    const auto keepAlive = 
std::chrono::duration_cast<std::chrono::duration<uint64_t>>(keep_alive_probe_);

Review comment:
       I think casting to std::chrono::seconds (which is actually 
duration<uint64_t>) is a bit more talkative.

##########
File path: extensions/http-curl/client/HTTPClient.h
##########
@@ -124,11 +135,11 @@ class HTTPClient : public BaseHTTPClient, public 
core::Connectable {
 
   bool setMinimumSSLVersion(SSLVersion minimum_version) override;
 
-  void setKeepAliveProbe(long probe){
+  void setKeepAliveProbe(std::chrono::milliseconds probe){
     keep_alive_probe_ = probe;
   }
 
-  void setKeepAliveIdle(long idle){

Review comment:
       Could we keep this as deprecated functions as well?

##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -445,4 +445,49 @@ class HeartbeatHandler : public CivetHandler {
   bool isSecure;
 };
 
+class InvokeHTTPResponseOKHandler : public CivetHandler {
+public:
+  bool handlePost(CivetServer *, struct mg_connection *conn) {
+    std::stringstream headers;
+    headers << "HTTP/1.1 201 OK\r\nContent-Type: text/plain\r\nContent-Length: 
0\r\nConnection: close\r\n\r\n";

Review comment:
       I don't think stringstream makes sense in case we only put one string to 
it. 
   Can we simply pass the hardcoded cstr to the mg_printf?

##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -445,4 +445,49 @@ class HeartbeatHandler : public CivetHandler {
   bool isSecure;
 };
 
+class InvokeHTTPResponseOKHandler : public CivetHandler {
+public:
+  bool handlePost(CivetServer *, struct mg_connection *conn) {
+    std::stringstream headers;
+    headers << "HTTP/1.1 201 OK\r\nContent-Type: text/plain\r\nContent-Length: 
0\r\nConnection: close\r\n\r\n";
+    mg_printf(conn, headers.str().c_str());
+    return true;
+  }
+};
+
+class InvokeHTTPResponse404Handler : public CivetHandler {
+public:
+  bool handlePost(CivetServer *, struct mg_connection *conn) {
+    std::stringstream headers;
+    headers << "HTTP/1.1 404 Not Found\r\nContent-Type: 
text/plain\r\nContent-Length: 0\r\nConnection: close\r\n\r\n";
+    mg_printf(conn, headers.str().c_str());
+    return true;
+  }
+};
+
+class InvokeHTTPResponse501Handler : public CivetHandler {
+public:
+  bool handlePost(CivetServer *, struct mg_connection *conn) {
+    std::stringstream headers;
+    headers << "HTTP/1.1 501 Not Implemented\r\nContent-Type: 
text/plain\r\nContent-Length: 0\r\nConnection: close\r\n\r\n";
+    mg_printf(conn, headers.str().c_str());
+    return true;
+  }
+};
+
+class InvokeHTTPResponseTimeoutHandler : public CivetHandler {
+public:
+    InvokeHTTPResponseTimeoutHandler(std::chrono::milliseconds wait_ms)
+        : wait_(wait_ms) {
+    }
+  bool handlePost(CivetServer *, struct mg_connection *conn) {
+    std::this_thread::sleep_for(wait_);

Review comment:
       Nice solution!

##########
File path: libminifi/test/resources/TestInvokeHTTPPost.yml
##########
@@ -0,0 +1,161 @@
+MiNiFi Config Version: 3
+Flow Controller:
+  name: c++lw
+  comment: Created by MiNiFi C2 Flow Designer
+Core Properties:
+  flow controller graceful shutdown period: 10 sec
+  flow service write delay interval: 500 ms
+  administrative yield duration: 30 sec
+  bored yield duration: 10 millis
+  max concurrent threads: 1
+  variable registry properties: ''
+FlowFile Repository:
+  partitions: 256
+  checkpoint interval: 2 mins
+  always sync: false
+  Swap:
+    threshold: 20000
+    in period: 5 sec
+    in threads: 1
+    out period: 5 sec
+    out threads: 4
+Content Repository:
+  content claim max appendable size: 10 MB
+  content claim max flow files: 100
+  always sync: false
+Provenance Repository:
+  provenance rollover time: 1 min
+  implementation: 
org.apache.nifi.provenance.MiNiFiPersistentProvenanceRepository
+Component Status Repository:
+  buffer size: 1440
+  snapshot frequency: 1 min
+Security Properties:
+  keystore: ''
+  keystore type: ''
+  keystore password: ''
+  key password: ''
+  truststore: ''
+  truststore type: ''
+  truststore password: ''
+  ssl protocol: ''
+  Sensitive Props:
+    key:
+    algorithm: PBEWITHMD5AND256BITAES-CBC-OPENSSL
+    provider: BC
+Processors:
+- id: 4ed2d51d-076a-49b0-88de-5cf5adf52a7e
+  name: GenerateFlowFile
+  class: org.apache.nifi.minifi.processors.GenerateFlowFile
+  max concurrent tasks: 1
+  scheduling strategy: TIMER_DRIVEN
+  scheduling period: 15000 ms
+  penalization period: 1000 ms
+  yield period: 1000 ms
+  run duration nanos: 0
+  auto-terminated relationships list: []
+  Properties:
+    Batch Size: '1'
+    Data Format: Binary
+    File Size: 1 kB
+    Unique FlowFiles: 'true'
+- id: 1d51724d-dd76-46a0-892d-a7c7408d58dd
+  name: InvokeHTTP
+  class: org.apache.nifi.minifi.processors.InvokeHTTP
+  max concurrent tasks: 1
+  scheduling strategy: TIMER_DRIVEN
+  scheduling period: 1000 ms
+  penalization period: 1000 ms
+  yield period: 1000 ms
+  run duration nanos: 0
+  auto-terminated relationships list: []
+  Properties:
+    Always Output Response: 'false'
+    Connection Timeout: 3 s
+    Content-type: application/octet-stream
+    Disable Peer Verification: 'false'
+    HTTP Method: POST
+    Include Date Header: 'true'
+    Read Timeout: 4 s
+    Remote URL: http://localhost:10033/minifi

Review comment:
       I don't really like using hardcoded magic ports and in my opinion we can 
avoid this.
   
   Civet server supports starting the webserver on random port (I think we 
already have some examples for that usage in our tests), and in 
queryRootProcessGroup we could override this property.
   The civet server should be started before this function executes, but I 
think that's already done in proper order. 




----------------------------------------------------------------
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:
[email protected]


Reply via email to