[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1225: MINIFICPP-1688: When storing time durations we should use std::chrono…

[GitBox <gi...@apache.org>]

szaszm commented on a change in pull request #1225:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1225#discussion_r778686770



##########
File path: extensions/mqtt/controllerservice/MQTTControllerService.h
##########
@@ -293,8 +289,8 @@ class MQTTControllerService : public core::controller::ControllerService {
   MQTTClient client_;
   std::string uri_;
   std::string topic_;
-  int64_t keepAliveInterval_;
-  int64_t connectionTimeOut_;
+  std::chrono::milliseconds keepAliveInterval_{0};
+  std::chrono::milliseconds connectionTimeOut_{0};

Review comment:
       Timeout is generally written as a single word, so I would rename this to `connectionTimeout_`

##########
File path: libminifi/test/unit/CpuUsageTest.cpp
##########
@@ -23,39 +23,39 @@
 #include "utils/ProcessCpuUsageTracker.h"
 #include "../TestBase.h"
 
-void busySleep(int duration_ms, std::chrono::milliseconds& start_ms, std::chrono::milliseconds& end_ms, const std::chrono::system_clock::time_point& origin) {
-  start_ms = std::chrono::duration_cast<std::chrono::milliseconds> (std::chrono::system_clock::now() - origin);
-  end_ms = std::chrono::duration_cast<std::chrono::milliseconds> (std::chrono::system_clock::now() - origin);
-  while (end_ms-start_ms < std::chrono::milliseconds(duration_ms)) {
-    end_ms = std::chrono::duration_cast<std::chrono::milliseconds> (std::chrono::system_clock::now() - origin);
+using namespace std::literals::chrono_literals;
+using namespace std::chrono;

Review comment:
       This should fail on the linter check. If it doesn't, it doesn't particularly bother me in a private test code, but I would consider a short namespace alias instead.

##########
File path: libminifi/include/utils/TimeUtil.h
##########
@@ -197,6 +193,91 @@ inline bool getDateTimeStr(int64_t unix_timestamp, std::string& date_time_str) {
   return true;
 }
 
+namespace details {
+
+template<class Duration>
+bool unit_matches(const std::string&) {
+  return false;
+}
+
+template<>
+inline bool unit_matches<std::chrono::nanoseconds>(const std::string& unit) {
+  return unit == "ns" || unit == "nano" || unit == "nanos" || unit == "nanoseconds";
+}
+
+template<>
+inline bool unit_matches<std::chrono::microseconds>(const std::string& unit) {
+  return unit == "us" || unit == "micro" || unit == "micros" || unit == "microseconds" || unit == "microsecond";
+}
+
+template<>
+inline bool unit_matches<std::chrono::milliseconds>(const std::string& unit) {
+  return unit == "msec" || unit == "ms" || unit == "millisecond" || unit == "milliseconds" || unit == "msecs" || unit == "millis" || unit == "milli";
+}
+
+template<>
+inline bool unit_matches<std::chrono::seconds>(const std::string& unit) {
+  return unit == "sec" || unit == "s" || unit == "second" || unit == "seconds" || unit == "secs";
+}
+
+template<>
+inline bool unit_matches<std::chrono::minutes>(const std::string& unit) {
+  return unit == "min" || unit == "m" || unit == "mins" || unit == "minute" || unit == "minutes";
+}
+
+template<>
+inline bool unit_matches<std::chrono::hours>(const std::string& unit) {
+  return unit == "h" || unit == "hr" || unit == "hour" || unit == "hrs" || unit == "hours";
+}
+
+template<>
+inline bool unit_matches<std::chrono::days>(const std::string& unit) {
+  return unit == "d" || unit == "day" || unit == "days";
+}
+
+
+template<class TargetDuration, class SourceDuration>
+std::optional<TargetDuration> cast_if_unit_matches(const std::string& unit, const int64_t value) {
+  if (unit_matches<SourceDuration>(unit)) {
+    return std::chrono::duration_cast<TargetDuration>(SourceDuration(value));
+  } else {
+    return std::nullopt;
+  }
+}
+
+template<class TargetDuration, typename... T>
+std::optional<TargetDuration> cast_to_matching_unit(std::string& unit, const int64_t value) {
+  std::optional<TargetDuration> result;
+  ((result = cast_if_unit_matches<TargetDuration, T>(unit, value)) || ...);
+  return result;
+}
+
+inline bool get_unit_and_value(const std::string& input, std::string& unit, int64_t& value) {
+  std::stringstream input_stream(input);
+  input_stream >> value >> unit;
+  std::transform(unit.begin(), unit.end(), unit.begin(), ::tolower);
+  return !input_stream.fail();
+}
+
+}  // namespace details
+
+template<class TargetDuration>
+std::optional<TargetDuration> StringToDuration(const std::string& input) {
+  std::string unit;
+  int64_t value;
+  if (!details::get_unit_and_value(input, unit, value))
+    return std::nullopt;
+
+  return details::cast_to_matching_unit<TargetDuration,
+    std::chrono::nanoseconds,
+    std::chrono::microseconds,
+    std::chrono::milliseconds,
+    std::chrono::seconds,
+    std::chrono::minutes,
+    std::chrono::hours,
+    std::chrono::days>(unit, value);
+}

Review comment:
       Consider making this (and all of the helpers) `constexpr`. It would allow you to test it with `static_assert`. It might have to take a `std::string_view` instead of `std::string`, depending on the level of C++20 support our compilers implement.

##########
File path: libminifi/src/c2/C2Agent.cpp
##########
@@ -161,12 +161,9 @@ void C2Agent::configure(const std::shared_ptr<Configure> &configure, bool reconf
   }
 
   if (configure->get("nifi.c2.agent.heartbeat.period", "c2.agent.heartbeat.period", heartbeat_period)) {
-    core::TimeUnit unit;
-
     try {
-      int64_t schedulingPeriod = 0;
-      if (core::Property::StringToTime(heartbeat_period, schedulingPeriod, unit) && core::Property::ConvertTimeUnitToMS(schedulingPeriod, unit, schedulingPeriod)) {
-        heart_beat_period_ = schedulingPeriod;
+      if (auto heartbeat_period_ms = utils::timeutils::StringToDuration<std::chrono::milliseconds>(heartbeat_period)) {
+        heart_beat_period_ = heartbeat_period_ms->count();

Review comment:
       Consider changing the `heart_beat_period_` member to milliseconds

##########
File path: libminifi/src/utils/Id.cpp
##########
@@ -42,6 +42,7 @@
 #endif
 
 #include "utils/StringUtils.h"
+#include "utils/TimeUtil.h"

Review comment:
       Is this used somewhere? There is no other addition to this file, so I think this might be a leftover change.

##########
File path: libminifi/include/utils/TimeUtil.h
##########
@@ -197,6 +193,91 @@ inline bool getDateTimeStr(int64_t unix_timestamp, std::string& date_time_str) {
   return true;
 }
 
+namespace details {
+
+template<class Duration>
+bool unit_matches(const std::string&) {
+  return false;
+}
+
+template<>
+inline bool unit_matches<std::chrono::nanoseconds>(const std::string& unit) {
+  return unit == "ns" || unit == "nano" || unit == "nanos" || unit == "nanoseconds";
+}
+
+template<>
+inline bool unit_matches<std::chrono::microseconds>(const std::string& unit) {
+  return unit == "us" || unit == "micro" || unit == "micros" || unit == "microseconds" || unit == "microsecond";
+}
+
+template<>
+inline bool unit_matches<std::chrono::milliseconds>(const std::string& unit) {
+  return unit == "msec" || unit == "ms" || unit == "millisecond" || unit == "milliseconds" || unit == "msecs" || unit == "millis" || unit == "milli";
+}
+
+template<>
+inline bool unit_matches<std::chrono::seconds>(const std::string& unit) {
+  return unit == "sec" || unit == "s" || unit == "second" || unit == "seconds" || unit == "secs";
+}
+
+template<>
+inline bool unit_matches<std::chrono::minutes>(const std::string& unit) {
+  return unit == "min" || unit == "m" || unit == "mins" || unit == "minute" || unit == "minutes";
+}
+
+template<>
+inline bool unit_matches<std::chrono::hours>(const std::string& unit) {
+  return unit == "h" || unit == "hr" || unit == "hour" || unit == "hrs" || unit == "hours";
+}
+
+template<>
+inline bool unit_matches<std::chrono::days>(const std::string& unit) {
+  return unit == "d" || unit == "day" || unit == "days";
+}
+
+
+template<class TargetDuration, class SourceDuration>
+std::optional<TargetDuration> cast_if_unit_matches(const std::string& unit, const int64_t value) {
+  if (unit_matches<SourceDuration>(unit)) {
+    return std::chrono::duration_cast<TargetDuration>(SourceDuration(value));
+  } else {
+    return std::nullopt;
+  }
+}
+
+template<class TargetDuration, typename... T>
+std::optional<TargetDuration> cast_to_matching_unit(std::string& unit, const int64_t value) {
+  std::optional<TargetDuration> result;
+  ((result = cast_if_unit_matches<TargetDuration, T>(unit, value)) || ...);
+  return result;
+}
+
+inline bool get_unit_and_value(const std::string& input, std::string& unit, int64_t& value) {
+  std::stringstream input_stream(input);
+  input_stream >> value >> unit;
+  std::transform(unit.begin(), unit.end(), unit.begin(), ::tolower);
+  return !input_stream.fail();
+}

Review comment:
       I would prefer a stricter parsing logic than istream.
   For example: find the space, before that it's the value, afterwards it's the unit. I think this would be equivalent with the old one.

##########
File path: extensions/mqtt/processors/AbstractMQTTProcessor.h
##########
@@ -65,7 +63,7 @@ class AbstractMQTTProcessor : public core::Processor {
       MQTTClient_unsubscribe(client_, topic_.c_str());
     }
     if (client_ && MQTTClient_isConnected(client_)) {
-      MQTTClient_disconnect(client_, connectionTimeOut_);
+      MQTTClient_disconnect(client_, std::chrono::duration_cast<std::chrono::milliseconds>(connectionTimeOut_).count());

Review comment:
       No casting is needed here. If you want to make it explicit that it's milliseconds, you can use a constructor call like this:
   ```suggestion
         MQTTClient_disconnect(client_, std::chrono::milliseconds{connectionTimeOut_}.count());
   ```
   But I think just `connectionTimeOut_.count()` is fine.




-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org