[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1631: MINIFICPP-2174 Send all cached compressed log files through C2

["fgerlits (via GitHub)" <gi...@apache.org>]

fgerlits commented on code in PR #1631:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1631#discussion_r1329969207


##########
libminifi/test/unit/LoggerTests.cpp:
##########
@@ -276,7 +280,7 @@ TEST_CASE("Setting either properties to 0 disables in-memory compressed logs", "
   log_config.initialize(properties);
   auto logger = log_config.getLogger("DisableCompressionTestLogger");
   logger->log_error("Hi there");
-  REQUIRE((logging::LoggerConfiguration::getCompressedLog(true) == nullptr) == is_nullptr);
+  REQUIRE(logging::LoggerConfiguration::getCompressedLogs(true).empty() == is_nullptr);

Review Comment:
   it would be better to rename `is_nullptr` to `is_empty`



##########
libminifi/include/core/logging/internal/LogCompressorSink.h:
##########
@@ -57,26 +58,44 @@ class LogCompressorSink : public spdlog::sinks::base_sink<std::mutex> {
   ~LogCompressorSink() override;
 
   template<class Rep, class Period>
-  std::unique_ptr<io::InputStream> getContent(const std::chrono::duration<Rep, Period>& time, bool flush = false) {
+  std::vector<std::unique_ptr<io::InputStream>> getContent(const std::chrono::duration<Rep, Period>& time, bool flush = false) {
     if (flush) {
       cached_logs_.commit();
       compress(true);
     }
-    LogBuffer compressed;
-    if (!compressed_logs_.tryDequeue(compressed, time) && flush) {
-      return createEmptyArchive();
+
+    std::vector<std::unique_ptr<io::InputStream>> log_segments;
+    const auto segment_count = compressed_logs_.itemCount();
+    for (size_t i = 0; i < segment_count; ++i) {
+      LogBuffer compressed;
+      if (!compressed_logs_.tryDequeue(compressed, time) && flush) {
+        break;

Review Comment:
   I don't understand the purpose of checking `flush` here (I didn't understand it before this change either, but it got more complex).  As far as I can see, we only call this with `flush = true`.
   
   Can we remove the `flush` argument?  If not, then I think the logic needs to be made clearer, either by a comment or some other way.



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