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 2022/06/16 13:49:57 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request, #1352: MINIFICPP-1842 getTimeStr should use std::chrono
martinzink opened a new pull request, #1352:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352
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 main)?
- [ ] 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 GitHub Actions CI results 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adam-markovics commented on a diff in pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
Posted by GitBox <gi...@apache.org>.
adam-markovics commented on code in PR #1352:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352#discussion_r901664241
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -323,8 +323,8 @@ bool ListSFTP::createAndTransferFlowFileFromChild(
logger_->log_error("Modification date %lu of \"%s/%s\" larger than int64_t max", child.attrs.mtime, child.parent_path.c_str(), child.filename.c_str());
return true;
}
- std::string mtime_str;
- if (!utils::timeutils::getDateTimeStr(gsl::narrow<int64_t>(child.attrs.mtime), mtime_str)) {
+ auto mtime_str = utils::timeutils::getDateTimeStr(date::sys_seconds{std::chrono::seconds(child.attrs.mtime)});
+ if (!mtime_str) {
logger_->log_error("Failed to convert modification date %lu of \"%s/%s\" to string", child.attrs.mtime, child.parent_path.c_str(), child.filename.c_str());
return true;
Review Comment:
Why return true in an error case? Also at line 324.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352
--
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
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1352:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352#discussion_r902239657
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -323,8 +323,8 @@ bool ListSFTP::createAndTransferFlowFileFromChild(
logger_->log_error("Modification date %lu of \"%s/%s\" larger than int64_t max", child.attrs.mtime, child.parent_path.c_str(), child.filename.c_str());
return true;
}
- std::string mtime_str;
- if (!utils::timeutils::getDateTimeStr(gsl::narrow<int64_t>(child.attrs.mtime), mtime_str)) {
+ auto mtime_str = utils::timeutils::getDateTimeStr(date::sys_seconds{std::chrono::seconds(child.attrs.mtime)});
+ if (!mtime_str) {
logger_->log_error("Failed to convert modification date %lu of \"%s/%s\" to string", child.attrs.mtime, child.parent_path.c_str(), child.filename.c_str());
return true;
Review Comment:
No idea to be honest, but you are right on first sight that seems unintentional.
But I think fixing that is out of the scope of this PR, since the caller yields and ends the onTrigger if this return false. It would probably need a bit more investigation/refactor to fix this. (This was introduced in 2019)
--
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
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1352:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352#discussion_r903635897
##########
libminifi/include/utils/TimeUtil.h:
##########
@@ -86,125 +86,49 @@ class SteadyClock : public Clock {
}
};
-/**
- * Returns a string based on TIME_FORMAT, converting
- * the parameter to a string
- * @param msec milliseconds since epoch
- * @returns string representing the time
- */
-inline std::string getTimeStr(uint64_t msec, bool enforce_locale = false) {
- char date[120];
- time_t second = (time_t) (msec / 1000);
- msec = msec % 1000;
- strftime(date, sizeof(date) / sizeof(*date), TIME_FORMAT, (enforce_locale == true ? gmtime(&second) : localtime(&second)));
-
- std::string ret = date;
- date[0] = '\0';
- sprintf(date, ".%03llu", (unsigned long long) msec); // NOLINT
-
- ret += date;
- return ret;
+inline std::string getTimeStr(std::chrono::system_clock::time_point tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, TIME_FORMAT, std::chrono::floor<std::chrono::milliseconds>(tp));
+ return stream.str();
}
-inline time_t mkgmtime(struct tm* date_time) {
-#ifdef WIN32
- return _mkgmtime(date_time);
-#else
- static const int month_lengths[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const int month_lengths_leap[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const auto is_leap_year = [](int year) -> bool {
- return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
- };
- static const auto num_leap_days = [](int year) -> int {
- return (year - 1968) / 4 - (year - 1900) / 100 + (year - 1600) / 400;
- };
-
- int year = date_time->tm_year + 1900;
- time_t result = year - 1970;
- result *= 365;
- result += num_leap_days(year - 1);
-
- for (int i = 0; i < 12 && i < date_time->tm_mon; ++i) {
- result += is_leap_year(year) ? month_lengths_leap[i] : month_lengths[i];
- }
-
- result += date_time->tm_mday - 1;
- result *= 24;
-
- result += date_time->tm_hour;
- result *= 60;
-
- result += date_time->tm_min;
- result *= 60;
-
- result += date_time->tm_sec;
- return result;
-#endif
+template<class TimePoint>
+inline std::optional<TimePoint> parseDateTimeStr(const std::string& str) {
Review Comment:
makes sense, removed the template in https://github.com/apache/nifi-minifi-cpp/pull/1352/commits/68929f499f8dc41248d06c564ca9ed21f84609a1#diff-4a76905d55704437ae0a7a4ee434a73f057c105c97bdf8756f5747b4fbc65e9fL105
##########
libminifi/test/unit/TimeUtilTests.cpp:
##########
@@ -21,79 +21,60 @@
using namespace std::literals::chrono_literals;
-namespace {
- constexpr int ONE_HOUR = 60 * 60;
- constexpr int ONE_DAY = 24 * ONE_HOUR;
-
- struct tm createTm(int year, int month, int day, int hour, int minute, int second, bool is_dst = false) {
- struct tm date_time;
- date_time.tm_year = year - 1900;
- date_time.tm_mon = month - 1;
- date_time.tm_mday = day;
- date_time.tm_hour = hour;
- date_time.tm_min = minute;
- date_time.tm_sec = second;
- date_time.tm_isdst = is_dst ? 1 : 0;
- return date_time;
- }
-
- void mkgmtimeTestHelper(time_t expected, int year, int month, int day, int hour, int minute, int second) {
- using org::apache::nifi::minifi::utils::timeutils::mkgmtime;
- struct tm date_time = createTm(year, month, day, hour, minute, second);
- REQUIRE(mkgmtime(&date_time) == expected);
- }
-} // namespace
+template <class TimePointType>
+void testParseDateTimeStr() {
+ using org::apache::nifi::minifi::utils::timeutils::parseDateTimeStr;
-TEST_CASE("mkgmtime() works correctly", "[mkgmtime]") {
- mkgmtimeTestHelper(0, 1970, 1, 1, 0, 0, 0);
- for (int hour = 0; hour < 24; ++hour) {
- mkgmtimeTestHelper((hour + 1) * ONE_HOUR - 1, 1970, 1, 1, hour, 59, 59);
- }
-
- mkgmtimeTestHelper(ONE_DAY, 1970, 1, 2, 0, 0, 0);
- mkgmtimeTestHelper(31 * ONE_DAY, 1970, 2, 1, 0, 0, 0);
- mkgmtimeTestHelper(365 * ONE_DAY, 1971, 1, 1, 0, 0, 0);
-
- mkgmtimeTestHelper(793929600, 1995, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(793929600 + ONE_DAY, 1995, 3, 1, 0, 0, 0);
- mkgmtimeTestHelper(825465600, 1996, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(825465600 + ONE_DAY, 1996, 2, 29, 0, 0, 0);
- mkgmtimeTestHelper(951696000, 2000, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(951696000 + ONE_DAY, 2000, 2, 29, 0, 0, 0);
- mkgmtimeTestHelper(4107456000, 2100, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(4107456000 + ONE_DAY, 2100, 3, 1, 0, 0, 0);
-
- mkgmtimeTestHelper(1513104856, 2017, 12, 12, 18, 54, 16);
- mkgmtimeTestHelper(1706655675, 2024, 1, 30, 23, 01, 15);
- mkgmtimeTestHelper(3710453630, 2087, 7, 31, 01, 33, 50);
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-01T00:00:00Z") == TimePointType{0s});
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-01T00:59:59Z") == TimePointType{1h - 1s});
+
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-02T00:00:00Z") == TimePointType{std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1970-02-01T00:00:00Z") == TimePointType{31 * std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1971-01-01T00:00:00Z") == TimePointType{365 * std::chrono::days(1)});
+
+ CHECK(*parseDateTimeStr<TimePointType>("1995-02-28T00:00:00Z") == TimePointType{793929600s});
+ CHECK(*parseDateTimeStr<TimePointType>("1995-03-01T00:00:00Z") == TimePointType{793929600s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1996-02-28T00:00:00Z") == TimePointType{825465600s});
+ CHECK(*parseDateTimeStr<TimePointType>("1996-02-29T00:00:00Z") == TimePointType{825465600s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("2000-02-28T00:00:00Z") == TimePointType{951696000s});
+ CHECK(*parseDateTimeStr<TimePointType>("2000-02-29T00:00:00Z") == TimePointType{951696000s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("2100-02-28T00:00:00Z") == TimePointType{4107456000s});
+ CHECK(*parseDateTimeStr<TimePointType>("2100-03-01T00:00:00Z") == TimePointType{4107456000s + std::chrono::days(1)});
+
+ CHECK(*parseDateTimeStr<TimePointType>("2017-12-12T18:54:16Z") == TimePointType{1513104856s});
+ CHECK(*parseDateTimeStr<TimePointType>("2024-01-30T23:01:15Z") == TimePointType{1706655675s});
+ CHECK(*parseDateTimeStr<TimePointType>("2087-07-31T01:33:50Z") == TimePointType{3710453630s});
}
TEST_CASE("parseDateTimeStr() works correctly", "[parseDateTimeStr]") {
Review Comment:
:+1: https://github.com/apache/nifi-minifi-cpp/pull/1352/commits/68929f499f8dc41248d06c564ca9ed21f84609a1#diff-821242a5468ace9f4c44a7d4c13d422cdea2d26de1ac3f6d1be4090e750427d4R48-R69
##########
libminifi/include/utils/TimeUtil.h:
##########
@@ -86,125 +86,49 @@ class SteadyClock : public Clock {
}
};
-/**
- * Returns a string based on TIME_FORMAT, converting
- * the parameter to a string
- * @param msec milliseconds since epoch
- * @returns string representing the time
- */
-inline std::string getTimeStr(uint64_t msec, bool enforce_locale = false) {
- char date[120];
- time_t second = (time_t) (msec / 1000);
- msec = msec % 1000;
- strftime(date, sizeof(date) / sizeof(*date), TIME_FORMAT, (enforce_locale == true ? gmtime(&second) : localtime(&second)));
-
- std::string ret = date;
- date[0] = '\0';
- sprintf(date, ".%03llu", (unsigned long long) msec); // NOLINT
-
- ret += date;
- return ret;
+inline std::string getTimeStr(std::chrono::system_clock::time_point tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, TIME_FORMAT, std::chrono::floor<std::chrono::milliseconds>(tp));
+ return stream.str();
}
-inline time_t mkgmtime(struct tm* date_time) {
-#ifdef WIN32
- return _mkgmtime(date_time);
-#else
- static const int month_lengths[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const int month_lengths_leap[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const auto is_leap_year = [](int year) -> bool {
- return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
- };
- static const auto num_leap_days = [](int year) -> int {
- return (year - 1968) / 4 - (year - 1900) / 100 + (year - 1600) / 400;
- };
-
- int year = date_time->tm_year + 1900;
- time_t result = year - 1970;
- result *= 365;
- result += num_leap_days(year - 1);
-
- for (int i = 0; i < 12 && i < date_time->tm_mon; ++i) {
- result += is_leap_year(year) ? month_lengths_leap[i] : month_lengths[i];
- }
-
- result += date_time->tm_mday - 1;
- result *= 24;
-
- result += date_time->tm_hour;
- result *= 60;
-
- result += date_time->tm_min;
- result *= 60;
-
- result += date_time->tm_sec;
- return result;
-#endif
+template<class TimePoint>
+inline std::optional<TimePoint> parseDateTimeStr(const std::string& str) {
+ std::istringstream stream(str);
+ TimePoint tp;
+ date::from_stream(stream, "%Y-%m-%dT%H:%M:%SZ", tp);
+ if (stream.fail() || (stream.peek() && !stream.eof()))
+ return std::nullopt;
+ return tp;
}
-/**
- * Parse a datetime in yyyy-MM-dd'T'HH:mm:ssZ format
- * @param str the datetime string
- * @returns Unix timestamp
- */
-inline int64_t parseDateTimeStr(const std::string& str) {
- /**
- * There is no strptime on Windows. As long as we have to parse a single date format this is not so bad,
- * but if multiple formats will have to be supported in the future, it might be worth it to include
- * an strptime implementation from some BSD on Windows.
- */
- uint32_t year;
- uint8_t month;
- uint8_t day;
- uint8_t hours;
- uint8_t minutes;
- uint8_t seconds;
- int read = 0;
- if (sscanf(str.c_str(), "%4u-%2hhu-%2hhuT%2hhu:%2hhu:%2hhuZ%n", &year, &month, &day, &hours, &minutes, &seconds, &read) != 6) {
- return -1;
- }
- // while it is unlikely that read will be < 0, the conditional adds little cost for a little defensiveness.
- if (read < 0 || static_cast<size_t>(read) != str.size()) {
- return -1;
- }
-
- if (year < 1970U ||
- month > 12U ||
- day > 31U ||
- hours > 23U ||
- minutes > 59U ||
- seconds > 60U) {
- return -1;
- }
-
- struct tm timeinfo{};
- timeinfo.tm_year = year - 1900;
- timeinfo.tm_mon = month - 1;
- timeinfo.tm_mday = day;
- timeinfo.tm_hour = hours;
- timeinfo.tm_min = minutes;
- timeinfo.tm_sec = seconds;
- timeinfo.tm_isdst = 0;
-
- return static_cast<int64_t>(mkgmtime(&timeinfo));
+template<class TimePoint>
+inline std::optional<std::string> getDateTimeStr(TimePoint tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, "%Y-%m-%dT%H:%M:%SZ", std::chrono::floor<std::chrono::milliseconds>(tp));
+ if (stream.fail())
+ return std::nullopt;
Review Comment:
you are right, this shouldnt fail, changed it in https://github.com/apache/nifi-minifi-cpp/pull/1352/commits/68929f499f8dc41248d06c564ca9ed21f84609a1#diff-4a76905d55704437ae0a7a4ee434a73f057c105c97bdf8756f5747b4fbc65e9fR104
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -63,6 +63,21 @@ constexpr char const* ListSFTP::TARGET_SYSTEM_TIMESTAMP_PRECISION_MINUTES;
constexpr char const* ListSFTP::ENTITY_TRACKING_INITIAL_LISTING_TARGET_TRACKING_TIME_WINDOW;
constexpr char const* ListSFTP::ENTITY_TRACKING_INITIAL_LISTING_TARGET_ALL_AVAILABLE;
+namespace {
+uint64_t createTimestamp(const std::optional<std::chrono::system_clock::time_point> time_point) {
+ if (!time_point)
+ return 0;
+ return std::chrono::duration_cast<std::chrono::milliseconds>(time_point->time_since_epoch()).count();
+}
+
+std::optional<std::chrono::system_clock::time_point> parseTimestamp(const uint64_t timestamp) {
+ if (timestamp == 0)
+ return std::nullopt;
+ return std::chrono::system_clock::time_point{std::chrono::milliseconds{timestamp}};
+}
Review Comment:
good idea, changed it in https://github.com/apache/nifi-minifi-cpp/pull/1352/commits/68929f499f8dc41248d06c564ca9ed21f84609a1#diff-2a26794f434d15c4c7911b91d7bf66a6e8069d49856da2b30c556439adb1b67cR67
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1352: MINIFICPP-1842 getTimeStr should use std::chrono
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1352:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1352#discussion_r903517228
##########
libminifi/test/unit/TimeUtilTests.cpp:
##########
@@ -21,79 +21,60 @@
using namespace std::literals::chrono_literals;
-namespace {
- constexpr int ONE_HOUR = 60 * 60;
- constexpr int ONE_DAY = 24 * ONE_HOUR;
-
- struct tm createTm(int year, int month, int day, int hour, int minute, int second, bool is_dst = false) {
- struct tm date_time;
- date_time.tm_year = year - 1900;
- date_time.tm_mon = month - 1;
- date_time.tm_mday = day;
- date_time.tm_hour = hour;
- date_time.tm_min = minute;
- date_time.tm_sec = second;
- date_time.tm_isdst = is_dst ? 1 : 0;
- return date_time;
- }
-
- void mkgmtimeTestHelper(time_t expected, int year, int month, int day, int hour, int minute, int second) {
- using org::apache::nifi::minifi::utils::timeutils::mkgmtime;
- struct tm date_time = createTm(year, month, day, hour, minute, second);
- REQUIRE(mkgmtime(&date_time) == expected);
- }
-} // namespace
+template <class TimePointType>
+void testParseDateTimeStr() {
+ using org::apache::nifi::minifi::utils::timeutils::parseDateTimeStr;
-TEST_CASE("mkgmtime() works correctly", "[mkgmtime]") {
- mkgmtimeTestHelper(0, 1970, 1, 1, 0, 0, 0);
- for (int hour = 0; hour < 24; ++hour) {
- mkgmtimeTestHelper((hour + 1) * ONE_HOUR - 1, 1970, 1, 1, hour, 59, 59);
- }
-
- mkgmtimeTestHelper(ONE_DAY, 1970, 1, 2, 0, 0, 0);
- mkgmtimeTestHelper(31 * ONE_DAY, 1970, 2, 1, 0, 0, 0);
- mkgmtimeTestHelper(365 * ONE_DAY, 1971, 1, 1, 0, 0, 0);
-
- mkgmtimeTestHelper(793929600, 1995, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(793929600 + ONE_DAY, 1995, 3, 1, 0, 0, 0);
- mkgmtimeTestHelper(825465600, 1996, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(825465600 + ONE_DAY, 1996, 2, 29, 0, 0, 0);
- mkgmtimeTestHelper(951696000, 2000, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(951696000 + ONE_DAY, 2000, 2, 29, 0, 0, 0);
- mkgmtimeTestHelper(4107456000, 2100, 2, 28, 0, 0, 0);
- mkgmtimeTestHelper(4107456000 + ONE_DAY, 2100, 3, 1, 0, 0, 0);
-
- mkgmtimeTestHelper(1513104856, 2017, 12, 12, 18, 54, 16);
- mkgmtimeTestHelper(1706655675, 2024, 1, 30, 23, 01, 15);
- mkgmtimeTestHelper(3710453630, 2087, 7, 31, 01, 33, 50);
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-01T00:00:00Z") == TimePointType{0s});
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-01T00:59:59Z") == TimePointType{1h - 1s});
+
+ CHECK(*parseDateTimeStr<TimePointType>("1970-01-02T00:00:00Z") == TimePointType{std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1970-02-01T00:00:00Z") == TimePointType{31 * std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1971-01-01T00:00:00Z") == TimePointType{365 * std::chrono::days(1)});
+
+ CHECK(*parseDateTimeStr<TimePointType>("1995-02-28T00:00:00Z") == TimePointType{793929600s});
+ CHECK(*parseDateTimeStr<TimePointType>("1995-03-01T00:00:00Z") == TimePointType{793929600s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("1996-02-28T00:00:00Z") == TimePointType{825465600s});
+ CHECK(*parseDateTimeStr<TimePointType>("1996-02-29T00:00:00Z") == TimePointType{825465600s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("2000-02-28T00:00:00Z") == TimePointType{951696000s});
+ CHECK(*parseDateTimeStr<TimePointType>("2000-02-29T00:00:00Z") == TimePointType{951696000s + std::chrono::days(1)});
+ CHECK(*parseDateTimeStr<TimePointType>("2100-02-28T00:00:00Z") == TimePointType{4107456000s});
+ CHECK(*parseDateTimeStr<TimePointType>("2100-03-01T00:00:00Z") == TimePointType{4107456000s + std::chrono::days(1)});
+
+ CHECK(*parseDateTimeStr<TimePointType>("2017-12-12T18:54:16Z") == TimePointType{1513104856s});
+ CHECK(*parseDateTimeStr<TimePointType>("2024-01-30T23:01:15Z") == TimePointType{1706655675s});
+ CHECK(*parseDateTimeStr<TimePointType>("2087-07-31T01:33:50Z") == TimePointType{3710453630s});
}
TEST_CASE("parseDateTimeStr() works correctly", "[parseDateTimeStr]") {
Review Comment:
I know it was missing before this PR, but it would be good to have some tests for `getDateTimeStr()`, too.
##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -63,6 +63,21 @@ constexpr char const* ListSFTP::TARGET_SYSTEM_TIMESTAMP_PRECISION_MINUTES;
constexpr char const* ListSFTP::ENTITY_TRACKING_INITIAL_LISTING_TARGET_TRACKING_TIME_WINDOW;
constexpr char const* ListSFTP::ENTITY_TRACKING_INITIAL_LISTING_TARGET_ALL_AVAILABLE;
+namespace {
+uint64_t createTimestamp(const std::optional<std::chrono::system_clock::time_point> time_point) {
+ if (!time_point)
+ return 0;
+ return std::chrono::duration_cast<std::chrono::milliseconds>(time_point->time_since_epoch()).count();
+}
+
+std::optional<std::chrono::system_clock::time_point> parseTimestamp(const uint64_t timestamp) {
+ if (timestamp == 0)
+ return std::nullopt;
+ return std::chrono::system_clock::time_point{std::chrono::milliseconds{timestamp}};
+}
Review Comment:
Minor quibble, but too many different things are called "timestamp" in this class. I would rename these helper functions to `toUnixTime` and `fromUnixTime`.
##########
libminifi/include/utils/TimeUtil.h:
##########
@@ -86,125 +86,49 @@ class SteadyClock : public Clock {
}
};
-/**
- * Returns a string based on TIME_FORMAT, converting
- * the parameter to a string
- * @param msec milliseconds since epoch
- * @returns string representing the time
- */
-inline std::string getTimeStr(uint64_t msec, bool enforce_locale = false) {
- char date[120];
- time_t second = (time_t) (msec / 1000);
- msec = msec % 1000;
- strftime(date, sizeof(date) / sizeof(*date), TIME_FORMAT, (enforce_locale == true ? gmtime(&second) : localtime(&second)));
-
- std::string ret = date;
- date[0] = '\0';
- sprintf(date, ".%03llu", (unsigned long long) msec); // NOLINT
-
- ret += date;
- return ret;
+inline std::string getTimeStr(std::chrono::system_clock::time_point tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, TIME_FORMAT, std::chrono::floor<std::chrono::milliseconds>(tp));
+ return stream.str();
}
-inline time_t mkgmtime(struct tm* date_time) {
-#ifdef WIN32
- return _mkgmtime(date_time);
-#else
- static const int month_lengths[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const int month_lengths_leap[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const auto is_leap_year = [](int year) -> bool {
- return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
- };
- static const auto num_leap_days = [](int year) -> int {
- return (year - 1968) / 4 - (year - 1900) / 100 + (year - 1600) / 400;
- };
-
- int year = date_time->tm_year + 1900;
- time_t result = year - 1970;
- result *= 365;
- result += num_leap_days(year - 1);
-
- for (int i = 0; i < 12 && i < date_time->tm_mon; ++i) {
- result += is_leap_year(year) ? month_lengths_leap[i] : month_lengths[i];
- }
-
- result += date_time->tm_mday - 1;
- result *= 24;
-
- result += date_time->tm_hour;
- result *= 60;
-
- result += date_time->tm_min;
- result *= 60;
-
- result += date_time->tm_sec;
- return result;
-#endif
+template<class TimePoint>
+inline std::optional<TimePoint> parseDateTimeStr(const std::string& str) {
Review Comment:
I'm not sure if it makes sense for `parseDateTimeStr` and `getDateTimeStr` to support `local_seconds` as well as `sys_seconds`. We never use them with `local_seconds`; the format for `local_seconds` would not have the `Z` at the end; and it is easy to convert `local_seconds` to or from `sys_seconds` if we need to.
I would change them to take and return `sys_seconds`, so they can be plain functions instead of templates.
##########
libminifi/include/utils/TimeUtil.h:
##########
@@ -86,125 +86,49 @@ class SteadyClock : public Clock {
}
};
-/**
- * Returns a string based on TIME_FORMAT, converting
- * the parameter to a string
- * @param msec milliseconds since epoch
- * @returns string representing the time
- */
-inline std::string getTimeStr(uint64_t msec, bool enforce_locale = false) {
- char date[120];
- time_t second = (time_t) (msec / 1000);
- msec = msec % 1000;
- strftime(date, sizeof(date) / sizeof(*date), TIME_FORMAT, (enforce_locale == true ? gmtime(&second) : localtime(&second)));
-
- std::string ret = date;
- date[0] = '\0';
- sprintf(date, ".%03llu", (unsigned long long) msec); // NOLINT
-
- ret += date;
- return ret;
+inline std::string getTimeStr(std::chrono::system_clock::time_point tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, TIME_FORMAT, std::chrono::floor<std::chrono::milliseconds>(tp));
+ return stream.str();
}
-inline time_t mkgmtime(struct tm* date_time) {
-#ifdef WIN32
- return _mkgmtime(date_time);
-#else
- static const int month_lengths[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const int month_lengths_leap[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
- static const auto is_leap_year = [](int year) -> bool {
- return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
- };
- static const auto num_leap_days = [](int year) -> int {
- return (year - 1968) / 4 - (year - 1900) / 100 + (year - 1600) / 400;
- };
-
- int year = date_time->tm_year + 1900;
- time_t result = year - 1970;
- result *= 365;
- result += num_leap_days(year - 1);
-
- for (int i = 0; i < 12 && i < date_time->tm_mon; ++i) {
- result += is_leap_year(year) ? month_lengths_leap[i] : month_lengths[i];
- }
-
- result += date_time->tm_mday - 1;
- result *= 24;
-
- result += date_time->tm_hour;
- result *= 60;
-
- result += date_time->tm_min;
- result *= 60;
-
- result += date_time->tm_sec;
- return result;
-#endif
+template<class TimePoint>
+inline std::optional<TimePoint> parseDateTimeStr(const std::string& str) {
+ std::istringstream stream(str);
+ TimePoint tp;
+ date::from_stream(stream, "%Y-%m-%dT%H:%M:%SZ", tp);
+ if (stream.fail() || (stream.peek() && !stream.eof()))
+ return std::nullopt;
+ return tp;
}
-/**
- * Parse a datetime in yyyy-MM-dd'T'HH:mm:ssZ format
- * @param str the datetime string
- * @returns Unix timestamp
- */
-inline int64_t parseDateTimeStr(const std::string& str) {
- /**
- * There is no strptime on Windows. As long as we have to parse a single date format this is not so bad,
- * but if multiple formats will have to be supported in the future, it might be worth it to include
- * an strptime implementation from some BSD on Windows.
- */
- uint32_t year;
- uint8_t month;
- uint8_t day;
- uint8_t hours;
- uint8_t minutes;
- uint8_t seconds;
- int read = 0;
- if (sscanf(str.c_str(), "%4u-%2hhu-%2hhuT%2hhu:%2hhu:%2hhuZ%n", &year, &month, &day, &hours, &minutes, &seconds, &read) != 6) {
- return -1;
- }
- // while it is unlikely that read will be < 0, the conditional adds little cost for a little defensiveness.
- if (read < 0 || static_cast<size_t>(read) != str.size()) {
- return -1;
- }
-
- if (year < 1970U ||
- month > 12U ||
- day > 31U ||
- hours > 23U ||
- minutes > 59U ||
- seconds > 60U) {
- return -1;
- }
-
- struct tm timeinfo{};
- timeinfo.tm_year = year - 1900;
- timeinfo.tm_mon = month - 1;
- timeinfo.tm_mday = day;
- timeinfo.tm_hour = hours;
- timeinfo.tm_min = minutes;
- timeinfo.tm_sec = seconds;
- timeinfo.tm_isdst = 0;
-
- return static_cast<int64_t>(mkgmtime(&timeinfo));
+template<class TimePoint>
+inline std::optional<std::string> getDateTimeStr(TimePoint tp) {
+ std::ostringstream stream;
+ date::to_stream(stream, "%Y-%m-%dT%H:%M:%SZ", std::chrono::floor<std::chrono::milliseconds>(tp));
+ if (stream.fail())
+ return std::nullopt;
Review Comment:
Is it possible for this to fail? `getTimeStr()` doesn't check for failure, which seems correct to me. If it can't fail, then the return value could be `string` instead of `optional<string>`.
--
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