You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/09 23:40:00 UTC
[GitHub] [geode-native] pdxcodemonkey opened a new pull request #761: GEODE-8679: switch to spdlog
pdxcodemonkey opened a new pull request #761:
URL: https://github.com/apache/geode-native/pull/761
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey closed pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey closed pull request #761:
URL: https://github.com/apache/geode-native/pull/761
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r631151376
##########
File path: cppcache/integration-test/LibraryCallbacks.cpp
##########
@@ -29,7 +29,7 @@ void dummyFunc() {}
} // namespace test
#define SLEEP(x) std::this_thread::sleep_for(std::chrono::milliseconds(x))
-#define LOG LOGDEBUG
+#define LOG LOG_DEBUG
Review comment:
Agree 100% in principle, but I think this is beyond the scope of the current PR. To give you an idea of the size of this task, here is the result of "find in files" for `LOG`, whole-word only, so I'd only catch test code that was using some variation of this hokey define:
`Matching lines: 2609 Matching files: 112 Total files searched: 1167`
That's a whooooole lot of logging to go remove from code that is targeted for deprecation.
--
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591922692
##########
File path: cppcache/integration-test/CMakeLists.txt
##########
@@ -30,6 +30,7 @@ target_link_libraries(test-cppcache-utils
PUBLIC
apache-geode
framework
+ SPDLog::SPDLog
Review comment:
removed
----------------------------------------------------------------
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] [geode-native] pivotal-jbarrett commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591920914
##########
File path: cppcache/src/Log.cpp
##########
@@ -45,9 +45,21 @@ const int LOG_SCRATCH_BUFFER_SIZE = 16 * __1K__;
static std::recursive_mutex g_logMutex;
static std::shared_ptr<spdlog::logger> currentLogger;
static LogLevel currentLevel = LogLevel::None;
+static std::string logFilePath;
+static int32_t adjustedFileSizeLimit;
+static int32_t maxFiles;
const std::shared_ptr<spdlog::logger>& getCurrentLogger() {
- return currentLogger;
+ if (logFilePath.empty()) {
Review comment:
In the Java bits we landed on `-`, consistent with lots of utilities, as the indicator to use `stdout` logging.
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591790825
##########
File path: CMakeLists.txt
##########
@@ -285,8 +285,9 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /ignore:4099")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /ignore:4099")
- # Enables multiprocess compiles
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP")
+ # /MP Enables multiprocess compiles
+ # SPDLOG_NO_TLS disables use of tls code which clashes with managed build on Windows
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP /DSPDLOG_NO_TLS")
Review comment:
Kind of looks like someone's bringing in a spdlog header file that shouldn't be. Will investigate.
----------------------------------------------------------------
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] [geode-native] lgtm-com[bot] commented on pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #761:
URL: https://github.com/apache/geode-native/pull/761#issuecomment-833813187
This pull request **introduces 1 alert** when merging 0b8e5fb4a715aa7f0fb6c22e860ee65ef19dcf41 into 36c84e2ce8b9f72b5c520e233bd991f1ac8d8894 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-4dbbfe2b6984963c1b3aa280b82a6d465383e19a)
**new alerts:**
* 1 for Constant return type on member
--
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] [geode-native] pivotal-jbarrett commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r590850026
##########
File path: CMakeLists.txt
##########
@@ -285,8 +285,9 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /ignore:4099")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /ignore:4099")
- # Enables multiprocess compiles
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP")
+ # /MP Enables multiprocess compiles
+ # SPDLOG_NO_TLS disables use of tls code which clashes with managed build on Windows
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP /DSPDLOG_NO_TLS")
Review comment:
This sort for definition specific to a dependency should be defined in that dependency.
##########
File path: cppcache/integration-test/CMakeLists.txt
##########
@@ -30,6 +30,7 @@ target_link_libraries(test-cppcache-utils
PUBLIC
apache-geode
framework
+ SPDLog::SPDLog
Review comment:
Why are the integration test dependent on the `SPDLog` library? It's use should be hidden at the integration level. The library should be statically compiled into the `apache-geode` library.
##########
File path: cppcache/src/Log.cpp
##########
@@ -107,334 +78,280 @@ void Log::init(LogLevel level, const char* logFileName, int32_t logFileLimit,
init(level, logFileNameString, logFileLimit, logDiskSpaceLimit);
}
-void Log::rollLogFile() {
- if (g_log) {
- fclose(g_log);
- g_log = nullptr;
- }
-
- auto rollFileName =
- (g_fullpath.parent_path() /
- (g_fullpath.stem().string() + "-" + std::to_string(g_rollIndex) +
- g_fullpath.extension().string()))
- .string();
- try {
- auto rollFile = boost::filesystem::path(rollFileName);
- boost::filesystem::rename(g_fullpath, rollFile);
- g_rollFiles[g_rollIndex] = rollFile;
- g_rollIndex++;
- } catch (const boost::filesystem::filesystem_error&) {
- throw IllegalStateException("Failed to roll log file");
- }
-}
-
-void Log::removeOldestRolledLogFile() {
- if (g_rollFiles.size()) {
- auto index = g_rollFiles.begin()->first;
- auto fileToRemove = g_rollFiles.begin()->second;
- auto fileSize = boost::filesystem::file_size(fileToRemove);
- boost::filesystem::remove(fileToRemove);
- g_rollFiles.erase(index);
- g_spaceUsed -= fileSize;
- } else {
- throw IllegalStateException(
- "Failed to free sufficient disk space for logs");
- }
-}
-
-void Log::calculateUsedDiskSpace() {
- g_spaceUsed = 0;
- if (boost::filesystem::exists(g_fullpath)) {
- g_spaceUsed = boost::filesystem::file_size(g_fullpath);
- for (auto const& item : g_rollFiles) {
- g_spaceUsed += boost::filesystem::file_size(item.second);
- }
- }
-}
+std::string Log::logLineFormat() {
+ std::stringstream format;
+ const size_t MINBUFSIZE = 128;
+ auto now = std::chrono::system_clock::now();
+ auto secs = std::chrono::system_clock::to_time_t(now);
+ auto tm_val = apache::geode::util::chrono::localtime(secs);
-void Log::buildRollFileMapping() {
- const auto filterstring = g_fullpath.stem().string() + "-(\\d+)\\.log$";
- const std::regex my_filter(filterstring);
-
- g_rollFiles.clear();
-
- boost::filesystem::directory_iterator end_itr;
- for (boost::filesystem::directory_iterator i(
- g_fullpath.parent_path().string());
- i != end_itr; ++i) {
- if (boost::filesystem::is_regular_file(i->status())) {
- std::string filename = i->path().filename().string();
- std::regex testPattern(filterstring);
- std::match_results<std::string::const_iterator> testMatches;
- if (std::regex_search(std::string::const_iterator(filename.begin()),
- filename.cend(), testMatches, testPattern)) {
- auto index = std::atoi(
- std::string(testMatches[1].first, testMatches[1].second).c_str());
- g_rollFiles[index] = i->path();
- }
- }
- }
-}
+ format << "[%l %Y/%m/%d %H:%M:%S.%f " << std::put_time(&tm_val, "%Z ")
+ << boost::asio::ip::host_name() << ":%P %t] %v";
-void Log::setRollFileIndex() {
- g_rollIndex = 0;
- if (g_rollFiles.size()) {
- g_rollIndex = g_rollFiles.rbegin()->first + 1;
- }
+ return format.str();
}
-void Log::setSizeLimits(int32_t logFileLimit, int64_t logDiskSpaceLimit) {
+void Log::setSizeLimits(int32_t logFileLimit, int64_t logDiskSpaceLimit,
+ int32_t& adjustedFileLimit,
+ int64_t& adjustedDiskLimit) {
validateSizeLimits(logFileLimit, logDiskSpaceLimit);
// Default to 10MB file limit and 1GB disk limit
if (logFileLimit == 0 && logDiskSpaceLimit == 0) {
- g_fileSizeLimit = 10 * __1M__;
- g_diskSpaceLimit = 1000 * __1M__;
+ adjustedFileLimit = 10 * __1M__;
+ adjustedDiskLimit = 1000 * __1M__;
}
// disk space specified but file size is defaulted. Just use a single
// log file, i.e. set file limit == disk limit
else if (logFileLimit == 0) {
- g_diskSpaceLimit = logDiskSpaceLimit * __1M__;
- g_fileSizeLimit = g_diskSpaceLimit;
+ adjustedDiskLimit = logDiskSpaceLimit * __1M__;
+ adjustedFileLimit = static_cast<int32_t>(adjustedDiskLimit);
} else if (logDiskSpaceLimit == 0) {
- g_fileSizeLimit = logFileLimit * __1M__;
- g_diskSpaceLimit = g_fileSizeLimit;
+ adjustedFileLimit = logFileLimit * __1M__;
+ adjustedDiskLimit = adjustedFileLimit;
} else {
- g_fileSizeLimit = logFileLimit * __1M__;
- g_diskSpaceLimit = logDiskSpaceLimit * __1M__;
+ adjustedFileLimit = logFileLimit * __1M__;
+ adjustedDiskLimit = logDiskSpaceLimit * __1M__;
+ }
+}
+
+uint32_t Log::calculateMaxFilesForSpaceLimit(uint64_t logDiskSpaceLimit,
+ uint32_t logFileSizeLimit) {
+ uint32_t maxFiles = 1;
+
+ maxFiles = static_cast<uint32_t>(logDiskSpaceLimit / logFileSizeLimit) - 1;
+ // Must specify at least 1!
+ maxFiles = maxFiles > 0 ? maxFiles : 0;
+
+ return maxFiles;
+}
+
+spdlog::level::level_enum geodeLogLevelToSpdlogLevel(LogLevel logLevel) {
+ auto level = spdlog::level::level_enum::off;
+ switch (logLevel) {
+ case LogLevel::None:
+ level = spdlog::level::level_enum::off;
+ break;
+ case LogLevel::Error:
+ level = spdlog::level::level_enum::err;
+ break;
+ case LogLevel::Warning:
+ level = spdlog::level::level_enum::warn;
+ break;
+ case LogLevel::Info:
+ level = spdlog::level::level_enum::info;
+ break;
+ case LogLevel::Default:
+ level = spdlog::level::level_enum::info;
+ break;
+ case LogLevel::Config:
+ level = spdlog::level::level_enum::info;
+ break;
+ case LogLevel::Fine:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::Finer:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::Finest:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::Debug:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::All:
+ level = spdlog::level::level_enum::debug;
+ break;
}
+
+ return level;
}
-void Log::init(LogLevel level, const std::string& logFileName,
- int32_t logFileLimit, int64_t logDiskSpaceLimit) {
- if (g_log != nullptr) {
- throw IllegalStateException(
- "The Log has already been initialized. "
- "Call Log::close() before calling Log::init again.");
+void Log::init(LogLevel logLevel, const std::string& logFilename,
+ uint32_t logFileSizeLimit, uint64_t logDiskSpaceLimit) {
+ if (logLevel == LogLevel::None) {
+ return;
}
- s_logLevel = level;
try {
std::lock_guard<decltype(g_logMutex)> guard(g_logMutex);
- g_hostName = boost::asio::ip::host_name();
+ if (logFilename.empty()) {
+ if (logFileSizeLimit || logDiskSpaceLimit) {
+ IllegalArgumentException ex(
+ "Cannot specify a file or disk space size limit without specifying "
+ "a log file name.");
+ throw ex;
+ }
+ currentLevel = logLevel;
+ // Check for multiple initialization
+ if (currentLogger && currentLogger->name() == "console") {
+ return;
+ } else {
+ close();
+ }
+ currentLogger = spdlog::stderr_color_mt("console");
+ currentLogger->set_level(geodeLogLevelToSpdlogLevel(currentLevel));
+ currentLogger->set_pattern(logLineFormat());
+ return;
+ } else if (logDiskSpaceLimit && logFileSizeLimit > logDiskSpaceLimit) {
+ IllegalArgumentException ex(
+ "File size limit must be smaller than disk space limit for "
+ "logging.");
+ throw ex;
+ }
- g_fullpath =
- boost::filesystem::absolute(boost::filesystem::path(logFileName));
+ int32_t adjustedFileSizeLimit;
Review comment:
spdlog has its own log rotation, why are we implementing our own?
##########
File path: cppcache/src/Log.cpp
##########
@@ -14,70 +14,41 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
#include "util/Log.hpp"
-#include <algorithm>
-#include <cctype>
-#include <chrono>
+#include <cstdarg>
#include <cstdio>
-#include <ctime>
-#include <map>
-#include <mutex>
-#include <regex>
#include <sstream>
#include <string>
-#include <thread>
-#include <utility>
#include <boost/asio/ip/host_name.hpp>
#include <boost/filesystem.hpp>
-#include <boost/process/environment.hpp>
#include <geode/ExceptionTypes.hpp>
#include <geode/util/LogLevel.hpp>
#include "geodeBanner.hpp"
+#include "spdlog/sinks/rotating_file_sink.h"
+#include "spdlog/sinks/stdout_color_sinks.h"
+#include "spdlog/spdlog.h"
#include "util/chrono/time_point.hpp"
-namespace {
-
-static size_t g_bytesWritten = 0;
-
-static size_t g_fileSizeLimit = GEODE_MAX_LOG_FILE_LIMIT;
-static size_t g_diskSpaceLimit = GEODE_MAX_LOG_DISK_LIMIT;
-
-static std::mutex g_logMutex;
-
-static int g_rollIndex = 0;
-static size_t g_spaceUsed = 0;
-
-static boost::filesystem::path g_fullpath;
-static std::map<int32_t, boost::filesystem::path> g_rollFiles;
-
-static FILE* g_log = nullptr;
-
-static std::string g_hostName;
-
-const int __1K__ = 1024;
-const int __1M__ = (__1K__ * __1K__);
-
-} // namespace
-
namespace apache {
namespace geode {
namespace client {
-LogLevel Log::s_logLevel = LogLevel::Default;
-
-/*****************************************************************************/
+const int __1K__ = 1024;
+const int __1M__ = __1K__ * __1K__;
+const int __1G__ = __1K__ * __1M__;
+const int LOG_SCRATCH_BUFFER_SIZE = 16 * __1K__;
-LogLevel Log::logLevel() { return s_logLevel; }
+static std::recursive_mutex g_logMutex;
Review comment:
Why still global variables in logging?
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -315,18 +315,14 @@ TEST_F(LoggingTest, logInit) {
apache::geode::client::Log::close();
boost::filesystem::remove("LoggingTest (#).log");
- // Init with invalid filename
+#ifdef WIN32
Review comment:
Strikes me that this should be its own test.
##########
File path: dependencies/spdlog/CMakeLists.txt
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+project( spdlog VERSION 1.3.1 LANGUAGES NONE )
+
+set( SHA256 db6986d0141546d4fba5220944cc1f251bd8afdfc434bda173b4b0b6406e3cd0 )
+
+include(GNUInstallDirs)
+include(ExternalProject)
+
+ExternalProject_Add( ${PROJECT_NAME}-extern
+ URL "https://github.com/gabime/spdlog/archive/v${PROJECT_VERSION}.zip"
+ URL_HASH SHA256=${SHA256}
+ UPDATE_COMMAND ""
+ CMAKE_ARGS
+ -DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}
+ -DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD}
+ -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
+ -DCMAKE_BUILD_TYPE=$<CONFIG>
+ -DSPDLOG_BUILD_BENCH=OFF
+ -DSPDLOG_BUILD_EXAMPLES=OFF
+ -DSPDLOG_BUILD_TESTS=OFF
+)
+
+ExternalProject_Get_Property( ${PROJECT_NAME}-extern INSTALL_DIR )
+
+add_library(${PROJECT_NAME} INTERFACE)
+
+target_include_directories(${PROJECT_NAME} SYSTEM INTERFACE
+ $<BUILD_INTERFACE:${INSTALL_DIR}/include>
+)
+
+target_link_libraries(${PROJECT_NAME} INTERFACE
+ spdlog
+)
+
+add_dependencies(${PROJECT_NAME} ${PROJECT_NAME}-extern)
+add_library(SPDLog::SPDLog ALIAS spdlog)
Review comment:
Given the project name, library name, namespace is "spdlog", I think this should probably be `spdlog::spdlog`. The CMake naming convention isn't super strong here but tends towards "project":"library" matching the case of the respective component.
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -103,7 +103,7 @@ class LoggingTest : public testing::Test {
const char* filename, int32_t rollIndex) {
auto baseName = boost::filesystem::path(filename).stem().string();
auto rolledPath =
- logdir / boost::filesystem::path(baseName + "-" +
+ logdir / boost::filesystem::path(baseName + "." +
Review comment:
By nature, any test that using the filesystem can't be a unit test. This test should be moved to integration/test or use some in-memory log sink.
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591789633
##########
File path: cppcache/integration-test/CMakeLists.txt
##########
@@ -30,6 +30,7 @@ target_link_libraries(test-cppcache-utils
PUBLIC
apache-geode
framework
+ SPDLog::SPDLog
Review comment:
This may or may not be vestigial - I'll have to check. spdlog is a header-only library, at least as we're using it, so this dependency just means we need to include one of its headers. But again, I'm not sure that's the case any more, so I will try to remove this ref.
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r631882082
##########
File path: cppcache/src/Log.cpp
##########
@@ -14,70 +14,41 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
#include "util/Log.hpp"
-#include <algorithm>
-#include <cctype>
-#include <chrono>
+#include <cstdarg>
#include <cstdio>
-#include <ctime>
-#include <map>
-#include <mutex>
-#include <regex>
#include <sstream>
#include <string>
-#include <thread>
-#include <utility>
#include <boost/asio/ip/host_name.hpp>
#include <boost/filesystem.hpp>
-#include <boost/process/environment.hpp>
#include <geode/ExceptionTypes.hpp>
#include <geode/util/LogLevel.hpp>
#include "geodeBanner.hpp"
+#include "spdlog/sinks/rotating_file_sink.h"
+#include "spdlog/sinks/stdout_color_sinks.h"
+#include "spdlog/spdlog.h"
#include "util/chrono/time_point.hpp"
-namespace {
-
-static size_t g_bytesWritten = 0;
-
-static size_t g_fileSizeLimit = GEODE_MAX_LOG_FILE_LIMIT;
-static size_t g_diskSpaceLimit = GEODE_MAX_LOG_DISK_LIMIT;
-
-static std::mutex g_logMutex;
-
-static int g_rollIndex = 0;
-static size_t g_spaceUsed = 0;
-
-static boost::filesystem::path g_fullpath;
-static std::map<int32_t, boost::filesystem::path> g_rollFiles;
-
-static FILE* g_log = nullptr;
-
-static std::string g_hostName;
-
-const int __1K__ = 1024;
-const int __1M__ = (__1K__ * __1K__);
-
-} // namespace
-
namespace apache {
namespace geode {
namespace client {
-LogLevel Log::s_logLevel = LogLevel::Default;
-
-/*****************************************************************************/
+const int __1K__ = 1024;
+const int __1M__ = __1K__ * __1K__;
+const int __1G__ = __1K__ * __1M__;
+const int LOG_SCRATCH_BUFFER_SIZE = 16 * __1K__;
-LogLevel Log::logLevel() { return s_logLevel; }
+static std::recursive_mutex g_logMutex;
Review comment:
I was able to eliminate most of these, but several are still needed due to the original implementation of our logging. To wit, we need a mutex, the shared pointer to the logger, the current level, and a global singleton to initialize the logger at startup.
--
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] [geode-native] mreddington commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
mreddington commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r631129082
##########
File path: cppcache/integration-test/LibraryCallbacks.cpp
##########
@@ -29,7 +29,7 @@ void dummyFunc() {}
} // namespace test
#define SLEEP(x) std::this_thread::sleep_for(std::chrono::milliseconds(x))
-#define LOG LOGDEBUG
+#define LOG LOG_DEBUG
Review comment:
Our tests shouldn't be using the NC library logger, there's just no need or justification for it. The test harness should be responsible for all reporting. If the mocks and stated expectations and assertions are inadequate to correctly deduce the nature of a failed test, that indicates the test is inadequate. The test harness provides all the facilities necessary to express expected sequences of operations and additional information. Each expectation and and assertion returns a stream object for logging additional information. For example:
`EXPECT_EQ(foo, bar) << "I BLAME ZOIDBERG!";`
There are other facilities for logging additional information, like the `Gfsh` command output, that will only appear in the case of a failure. The test harness should be quiet, with a clean indication of success, and fail with a complete report. The harness knows how to do it. I recommend you map `LOG` to a no-op, and we begin removing all logging from tests.
##########
File path: dependencies/spdlog/CMakeLists.txt
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+project( spdlog VERSION 1.3.1 LANGUAGES NONE )
+
+set( SHA256 db6986d0141546d4fba5220944cc1f251bd8afdfc434bda173b4b0b6406e3cd0 )
+
+include(GNUInstallDirs)
+include(ExternalProject)
+
+ExternalProject_Add( ${PROJECT_NAME}-extern
+ URL "https://github.com/gabime/spdlog/archive/v${PROJECT_VERSION}.zip"
Review comment:
Is there a way we can track the tip of spdlog? Is that desirable?
--
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591788176
##########
File path: cppcache/src/Log.cpp
##########
@@ -107,334 +78,280 @@ void Log::init(LogLevel level, const char* logFileName, int32_t logFileLimit,
init(level, logFileNameString, logFileLimit, logDiskSpaceLimit);
}
-void Log::rollLogFile() {
- if (g_log) {
- fclose(g_log);
- g_log = nullptr;
- }
-
- auto rollFileName =
- (g_fullpath.parent_path() /
- (g_fullpath.stem().string() + "-" + std::to_string(g_rollIndex) +
- g_fullpath.extension().string()))
- .string();
- try {
- auto rollFile = boost::filesystem::path(rollFileName);
- boost::filesystem::rename(g_fullpath, rollFile);
- g_rollFiles[g_rollIndex] = rollFile;
- g_rollIndex++;
- } catch (const boost::filesystem::filesystem_error&) {
- throw IllegalStateException("Failed to roll log file");
- }
-}
-
-void Log::removeOldestRolledLogFile() {
- if (g_rollFiles.size()) {
- auto index = g_rollFiles.begin()->first;
- auto fileToRemove = g_rollFiles.begin()->second;
- auto fileSize = boost::filesystem::file_size(fileToRemove);
- boost::filesystem::remove(fileToRemove);
- g_rollFiles.erase(index);
- g_spaceUsed -= fileSize;
- } else {
- throw IllegalStateException(
- "Failed to free sufficient disk space for logs");
- }
-}
-
-void Log::calculateUsedDiskSpace() {
- g_spaceUsed = 0;
- if (boost::filesystem::exists(g_fullpath)) {
- g_spaceUsed = boost::filesystem::file_size(g_fullpath);
- for (auto const& item : g_rollFiles) {
- g_spaceUsed += boost::filesystem::file_size(item.second);
- }
- }
-}
+std::string Log::logLineFormat() {
+ std::stringstream format;
+ const size_t MINBUFSIZE = 128;
+ auto now = std::chrono::system_clock::now();
+ auto secs = std::chrono::system_clock::to_time_t(now);
+ auto tm_val = apache::geode::util::chrono::localtime(secs);
-void Log::buildRollFileMapping() {
- const auto filterstring = g_fullpath.stem().string() + "-(\\d+)\\.log$";
- const std::regex my_filter(filterstring);
-
- g_rollFiles.clear();
-
- boost::filesystem::directory_iterator end_itr;
- for (boost::filesystem::directory_iterator i(
- g_fullpath.parent_path().string());
- i != end_itr; ++i) {
- if (boost::filesystem::is_regular_file(i->status())) {
- std::string filename = i->path().filename().string();
- std::regex testPattern(filterstring);
- std::match_results<std::string::const_iterator> testMatches;
- if (std::regex_search(std::string::const_iterator(filename.begin()),
- filename.cend(), testMatches, testPattern)) {
- auto index = std::atoi(
- std::string(testMatches[1].first, testMatches[1].second).c_str());
- g_rollFiles[index] = i->path();
- }
- }
- }
-}
+ format << "[%l %Y/%m/%d %H:%M:%S.%f " << std::put_time(&tm_val, "%Z ")
+ << boost::asio::ip::host_name() << ":%P %t] %v";
-void Log::setRollFileIndex() {
- g_rollIndex = 0;
- if (g_rollFiles.size()) {
- g_rollIndex = g_rollFiles.rbegin()->first + 1;
- }
+ return format.str();
}
-void Log::setSizeLimits(int32_t logFileLimit, int64_t logDiskSpaceLimit) {
+void Log::setSizeLimits(int32_t logFileLimit, int64_t logDiskSpaceLimit,
+ int32_t& adjustedFileLimit,
+ int64_t& adjustedDiskLimit) {
validateSizeLimits(logFileLimit, logDiskSpaceLimit);
// Default to 10MB file limit and 1GB disk limit
if (logFileLimit == 0 && logDiskSpaceLimit == 0) {
- g_fileSizeLimit = 10 * __1M__;
- g_diskSpaceLimit = 1000 * __1M__;
+ adjustedFileLimit = 10 * __1M__;
+ adjustedDiskLimit = 1000 * __1M__;
}
// disk space specified but file size is defaulted. Just use a single
// log file, i.e. set file limit == disk limit
else if (logFileLimit == 0) {
- g_diskSpaceLimit = logDiskSpaceLimit * __1M__;
- g_fileSizeLimit = g_diskSpaceLimit;
+ adjustedDiskLimit = logDiskSpaceLimit * __1M__;
+ adjustedFileLimit = static_cast<int32_t>(adjustedDiskLimit);
} else if (logDiskSpaceLimit == 0) {
- g_fileSizeLimit = logFileLimit * __1M__;
- g_diskSpaceLimit = g_fileSizeLimit;
+ adjustedFileLimit = logFileLimit * __1M__;
+ adjustedDiskLimit = adjustedFileLimit;
} else {
- g_fileSizeLimit = logFileLimit * __1M__;
- g_diskSpaceLimit = logDiskSpaceLimit * __1M__;
+ adjustedFileLimit = logFileLimit * __1M__;
+ adjustedDiskLimit = logDiskSpaceLimit * __1M__;
+ }
+}
+
+uint32_t Log::calculateMaxFilesForSpaceLimit(uint64_t logDiskSpaceLimit,
+ uint32_t logFileSizeLimit) {
+ uint32_t maxFiles = 1;
+
+ maxFiles = static_cast<uint32_t>(logDiskSpaceLimit / logFileSizeLimit) - 1;
+ // Must specify at least 1!
+ maxFiles = maxFiles > 0 ? maxFiles : 0;
+
+ return maxFiles;
+}
+
+spdlog::level::level_enum geodeLogLevelToSpdlogLevel(LogLevel logLevel) {
+ auto level = spdlog::level::level_enum::off;
+ switch (logLevel) {
+ case LogLevel::None:
+ level = spdlog::level::level_enum::off;
+ break;
+ case LogLevel::Error:
+ level = spdlog::level::level_enum::err;
+ break;
+ case LogLevel::Warning:
+ level = spdlog::level::level_enum::warn;
+ break;
+ case LogLevel::Info:
+ level = spdlog::level::level_enum::info;
+ break;
+ case LogLevel::Default:
+ level = spdlog::level::level_enum::info;
+ break;
+ case LogLevel::Config:
+ level = spdlog::level::level_enum::info;
+ break;
+ case LogLevel::Fine:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::Finer:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::Finest:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::Debug:
+ level = spdlog::level::level_enum::debug;
+ break;
+ case LogLevel::All:
+ level = spdlog::level::level_enum::debug;
+ break;
}
+
+ return level;
}
-void Log::init(LogLevel level, const std::string& logFileName,
- int32_t logFileLimit, int64_t logDiskSpaceLimit) {
- if (g_log != nullptr) {
- throw IllegalStateException(
- "The Log has already been initialized. "
- "Call Log::close() before calling Log::init again.");
+void Log::init(LogLevel logLevel, const std::string& logFilename,
+ uint32_t logFileSizeLimit, uint64_t logDiskSpaceLimit) {
+ if (logLevel == LogLevel::None) {
+ return;
}
- s_logLevel = level;
try {
std::lock_guard<decltype(g_logMutex)> guard(g_logMutex);
- g_hostName = boost::asio::ip::host_name();
+ if (logFilename.empty()) {
+ if (logFileSizeLimit || logDiskSpaceLimit) {
+ IllegalArgumentException ex(
+ "Cannot specify a file or disk space size limit without specifying "
+ "a log file name.");
+ throw ex;
+ }
+ currentLevel = logLevel;
+ // Check for multiple initialization
+ if (currentLogger && currentLogger->name() == "console") {
+ return;
+ } else {
+ close();
+ }
+ currentLogger = spdlog::stderr_color_mt("console");
+ currentLogger->set_level(geodeLogLevelToSpdlogLevel(currentLevel));
+ currentLogger->set_pattern(logLineFormat());
+ return;
+ } else if (logDiskSpaceLimit && logFileSizeLimit > logDiskSpaceLimit) {
+ IllegalArgumentException ex(
+ "File size limit must be smaller than disk space limit for "
+ "logging.");
+ throw ex;
+ }
- g_fullpath =
- boost::filesystem::absolute(boost::filesystem::path(logFileName));
+ int32_t adjustedFileSizeLimit;
Review comment:
We're not... really. Our two existing properties are file size and disk space, specified in MB. spdlog uses two properties, file size and max number of files, specified in bytes. So we calculate limits like we did before, then translate disk space to a max number of files and give those limits to the spdlog file sink. There's no logic anymore in our code to check file size, roll logs, delete old files, etc.
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r631892232
##########
File path: dependencies/spdlog/CMakeLists.txt
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+project( spdlog VERSION 1.3.1 LANGUAGES NONE )
+
+set( SHA256 db6986d0141546d4fba5220944cc1f251bd8afdfc434bda173b4b0b6406e3cd0 )
+
+include(GNUInstallDirs)
+include(ExternalProject)
+
+ExternalProject_Add( ${PROJECT_NAME}-extern
+ URL "https://github.com/gabime/spdlog/archive/v${PROJECT_VERSION}.zip"
Review comment:
Dunno, but for sure we're already way out of date. Current version is 1.8.5, will update.
--
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r631882854
##########
File path: cppcache/src/Log.cpp
##########
@@ -45,9 +45,21 @@ const int LOG_SCRATCH_BUFFER_SIZE = 16 * __1K__;
static std::recursive_mutex g_logMutex;
static std::shared_ptr<spdlog::logger> currentLogger;
static LogLevel currentLevel = LogLevel::None;
+static std::string logFilePath;
+static int32_t adjustedFileSizeLimit;
+static int32_t maxFiles;
const std::shared_ptr<spdlog::logger>& getCurrentLogger() {
- return currentLogger;
+ if (logFilePath.empty()) {
Review comment:
Modified the code to accept this as an explicit request for `stdout` logging.
--
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r631918182
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -315,18 +315,14 @@ TEST_F(LoggingTest, logInit) {
apache::geode::client::Log::close();
boost::filesystem::remove("LoggingTest (#).log");
- // Init with invalid filename
+#ifdef WIN32
Review comment:
Moved file/disk space init and filename init tests to their own test cases.
--
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591922542
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -103,7 +103,7 @@ class LoggingTest : public testing::Test {
const char* filename, int32_t rollIndex) {
auto baseName = boost::filesystem::path(filename).stem().string();
auto rolledPath =
- logdir / boost::filesystem::path(baseName + "-" +
+ logdir / boost::filesystem::path(baseName + "." +
Review comment:
Integration tests should only be using the public API and dynamically linking to geode-native. The logger is not a public API. I'm unsure how I would go about testing file size and disk space limits using an in-memory sink - did you have something in mind?
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey commented on a change in pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #761:
URL: https://github.com/apache/geode-native/pull/761#discussion_r591919212
##########
File path: CMakeLists.txt
##########
@@ -285,8 +285,9 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /ignore:4099")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /ignore:4099")
- # Enables multiprocess compiles
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP")
+ # /MP Enables multiprocess compiles
+ # SPDLOG_NO_TLS disables use of tls code which clashes with managed build on Windows
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /MP /DSPDLOG_NO_TLS")
Review comment:
Removed.
----------------------------------------------------------------
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] [geode-native] pdxcodemonkey commented on pull request #761: GEODE-8679: switch to spdlog
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #761:
URL: https://github.com/apache/geode-native/pull/761#issuecomment-1031631405
There remains one integration test in the old suite (I believe) that hangs every time when run with DEBUG-level logging and this change. I don't have bandwidth or inclination to debug this issue to resolution. I'm also a lot more reluctant to introduce a 3rd-party logger into our code base, given everyone's recent experience with Log4j issues.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org