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/10 00:22:42 UTC
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #761: GEODE-8679: switch to spdlog
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