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/01/10 22:29:06 UTC
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #718: GEODE-8806: Refactor Logger code
pivotal-jbarrett commented on a change in pull request #718:
URL: https://github.com/apache/geode-native/pull/718#discussion_r554630927
##########
File path: cppcache/integration-test/fw_helper.hpp
##########
@@ -131,10 +131,7 @@ class TestException {
m_filename(const_cast<char*>(filename)) {}
void print() {
- char buf[256];
- apache::geode::client::Log::formatLogLine(
- buf, apache::geode::client::LogLevel::Error);
- fprintf(stdout, "--->%sTestException: %s in %s at line %d<---\n", buf,
+ fprintf(stdout, "--->%sTestException: %s in %s at line %d<---\n", apache::geode::client::Log::formatLogLine(apache::geode::client::LogLevel::Error).c_str(),
Review comment:
Why is the logic of this method inverted? Before the Log::formatLogLine formatted the message, now it only formats an empty string? Very confused.
##########
File path: cppcache/integration/test/CqPlusAuthInitializeTest.cpp
##########
@@ -64,8 +64,7 @@ const int32_t CQ_PLUS_AUTH_TEST_REGION_ENTRY_COUNT = 50000;
Cache createCache(std::shared_ptr<SimpleAuthInitialize> auth) {
auto cache = CacheFactory()
- .set("log-level", "debug")
- .set("log-file", "geode_native.log")
+ .set("log-level", "none")
Review comment:
Thank you for this!!!!
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -0,0 +1,759 @@
+/*
Review comment:
Yay for new test!
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -0,0 +1,759 @@
+/*
+ * 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.
+ */
+
+#include <map>
+#include <string>
+#include <util/Log.hpp>
+
+#include <boost/filesystem.hpp>
+#include <boost/regex.hpp>
+
+#include <gtest/gtest.h>
+
+#include <geode/AuthenticatedView.hpp>
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+using apache::geode::client::CacheClosedException;
+using apache::geode::client::CacheFactory;
+using apache::geode::client::LogLevel;
+using apache::geode::client::RegionShortcut;
+
+namespace {
+
+const auto __1K__ = 1024;
+const auto __4K__ = 4 * __1K__;
+const auto __1M__ = (__1K__ * __1K__);
+const auto __1G__ = (__1K__ * __1K__ * __1K__);
+
+const auto LENGTH_OF_BANNER = 16;
+
+auto testLogFileName = std::string("LoggingTest.log");
+
+const char* __1KStringLiteral =
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
+
+class LoggingTest : public testing::Test {
+ void scrubTestLogFiles() {
+ // Close logger, just in case
+ apache::geode::client::Log::close();
+
+ if (boost::filesystem::exists(testLogFileName)) {
+ boost::filesystem::remove(testLogFileName);
+ }
+
+ std::map<int32_t, boost::filesystem::path> rolledFiles;
+ LoggingTest::findRolledFiles(boost::filesystem::current_path().string(),
+ rolledFiles);
+ for (auto& item : rolledFiles) {
+ boost::filesystem::remove(item.second);
+ }
+ }
+
+ virtual void SetUp() { scrubTestLogFiles(); }
+
+ virtual void TearDown() { scrubTestLogFiles(); }
+
+ public:
+ static void writeRolledLogFile(const boost::filesystem::path& logdir,
+ int32_t rollIndex) {
+ auto rolledPath =
+ logdir / boost::filesystem::path("LoggingTest-" +
+ std::to_string(rollIndex) + ".log");
+ auto rolledFile = fopen(rolledPath.string().c_str(), "w");
+ fwrite("Test", 1, 4, rolledFile);
+ fclose(rolledFile);
+ }
+
+ static int numOfLinesInFile(const char* fname) {
+ char line[2048];
+ char* read;
+ int ln_cnt = 0;
+ FILE* fp = fopen(fname, "r");
+ if (fp == nullptr) {
+ return 0;
+ }
+ while (!!(read = fgets(line, sizeof line, fp))) {
+ ++ln_cnt;
+ }
+
+ if (!feof(fp)) {
+ fclose(fp);
+ return -2;
+ }
+ fclose(fp);
+ return ln_cnt;
+ }
+
+ static int expected(LogLevel level) {
+ int expected = static_cast<int>(level);
+ if (level >= LogLevel::Default) {
+ expected--;
+ }
+ return expected;
+ }
+
+ static int expectedWithBanner(LogLevel level) {
+ int expected = LoggingTest::expected(level);
+ if (level != LogLevel::None) {
+ expected += LENGTH_OF_BANNER;
+ }
+ return expected;
+ }
+
+ static void verifyLineCountAtLevel(LogLevel level) {
+ apache::geode::client::Log::init(level, testLogFileName);
+
+ apache::geode::client::Log::error("Error Message");
+ apache::geode::client::Log::warning("Warning Message");
+ apache::geode::client::Log::info("Info Message");
+ apache::geode::client::Log::config("Config Message");
+ apache::geode::client::Log::fine("Fine Message");
+ apache::geode::client::Log::finer("Finer Message");
+ apache::geode::client::Log::finest("Finest Message");
+ apache::geode::client::Log::debug("Debug Message");
+
+ int lines = LoggingTest::numOfLinesInFile(testLogFileName.c_str());
+
+ ASSERT_TRUE(lines == LoggingTest::expectedWithBanner(level));
+
+ apache::geode::client::Log::close();
+ boost::filesystem::remove(testLogFileName.c_str());
+ }
+
+ static void findRolledFiles(
+ const std::string& logFilePath,
+ std::map<int32_t, boost::filesystem::path>& rolledFiles) {
+ const auto basePath =
+ boost::filesystem::absolute(boost::filesystem::path(logFilePath)) /
+ testLogFileName;
+ const auto filterstring = basePath.stem().string() + "-(\\d+)\\.log$";
+ const boost::regex my_filter(filterstring);
Review comment:
Why `boost::regex` and not `std::regex`?
##########
File path: cppcache/integration/test/AuthInitializeTest.cpp
##########
@@ -68,7 +68,7 @@ const int32_t CQ_PLUS_AUTH_TEST_REGION_ENTRY_COUNT = 100000;
Cache createCache(std::shared_ptr<SimpleAuthInitialize> auth) {
auto cache = CacheFactory()
- .set("log-level", "debug")
+ .set("log-level", "none")
Review comment:
Since this test is explicitly setting a log file I suspect it might be slurping the logs for its test. If it is, bad but we shouldn't set to `none` here and break the test. If it isn't slurping the logs then remove the log `log-file` lines too.
##########
File path: cppcache/test/LoggingTest.cpp
##########
@@ -0,0 +1,759 @@
+/*
+ * 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.
+ */
+
+#include <map>
+#include <string>
+#include <util/Log.hpp>
+
+#include <boost/filesystem.hpp>
+#include <boost/regex.hpp>
+
+#include <gtest/gtest.h>
+
+#include <geode/AuthenticatedView.hpp>
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+using apache::geode::client::CacheClosedException;
+using apache::geode::client::CacheFactory;
+using apache::geode::client::LogLevel;
+using apache::geode::client::RegionShortcut;
+
+namespace {
+
+const auto __1K__ = 1024;
+const auto __4K__ = 4 * __1K__;
+const auto __1M__ = (__1K__ * __1K__);
+const auto __1G__ = (__1K__ * __1K__ * __1K__);
+
+const auto LENGTH_OF_BANNER = 16;
+
+auto testLogFileName = std::string("LoggingTest.log");
+
+const char* __1KStringLiteral =
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+ "AA"
+ "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
+
+class LoggingTest : public testing::Test {
+ void scrubTestLogFiles() {
+ // Close logger, just in case
+ apache::geode::client::Log::close();
+
+ if (boost::filesystem::exists(testLogFileName)) {
+ boost::filesystem::remove(testLogFileName);
+ }
+
+ std::map<int32_t, boost::filesystem::path> rolledFiles;
+ LoggingTest::findRolledFiles(boost::filesystem::current_path().string(),
+ rolledFiles);
+ for (auto& item : rolledFiles) {
+ boost::filesystem::remove(item.second);
+ }
+ }
+
+ virtual void SetUp() { scrubTestLogFiles(); }
+
+ virtual void TearDown() { scrubTestLogFiles(); }
+
+ public:
+ static void writeRolledLogFile(const boost::filesystem::path& logdir,
+ int32_t rollIndex) {
+ auto rolledPath =
+ logdir / boost::filesystem::path("LoggingTest-" +
+ std::to_string(rollIndex) + ".log");
+ auto rolledFile = fopen(rolledPath.string().c_str(), "w");
+ fwrite("Test", 1, 4, rolledFile);
+ fclose(rolledFile);
+ }
+
+ static int numOfLinesInFile(const char* fname) {
+ char line[2048];
+ char* read;
+ int ln_cnt = 0;
+ FILE* fp = fopen(fname, "r");
+ if (fp == nullptr) {
+ return 0;
+ }
+ while (!!(read = fgets(line, sizeof line, fp))) {
+ ++ln_cnt;
+ }
+
+ if (!feof(fp)) {
+ fclose(fp);
+ return -2;
+ }
+ fclose(fp);
+ return ln_cnt;
+ }
+
+ static int expected(LogLevel level) {
+ int expected = static_cast<int>(level);
+ if (level >= LogLevel::Default) {
+ expected--;
+ }
+ return expected;
+ }
+
+ static int expectedWithBanner(LogLevel level) {
+ int expected = LoggingTest::expected(level);
+ if (level != LogLevel::None) {
+ expected += LENGTH_OF_BANNER;
+ }
+ return expected;
+ }
+
+ static void verifyLineCountAtLevel(LogLevel level) {
+ apache::geode::client::Log::init(level, testLogFileName);
+
+ apache::geode::client::Log::error("Error Message");
+ apache::geode::client::Log::warning("Warning Message");
+ apache::geode::client::Log::info("Info Message");
+ apache::geode::client::Log::config("Config Message");
+ apache::geode::client::Log::fine("Fine Message");
+ apache::geode::client::Log::finer("Finer Message");
+ apache::geode::client::Log::finest("Finest Message");
+ apache::geode::client::Log::debug("Debug Message");
+
+ int lines = LoggingTest::numOfLinesInFile(testLogFileName.c_str());
+
+ ASSERT_TRUE(lines == LoggingTest::expectedWithBanner(level));
+
+ apache::geode::client::Log::close();
+ boost::filesystem::remove(testLogFileName.c_str());
+ }
+
+ static void findRolledFiles(
+ const std::string& logFilePath,
+ std::map<int32_t, boost::filesystem::path>& rolledFiles) {
+ const auto basePath =
+ boost::filesystem::absolute(boost::filesystem::path(logFilePath)) /
+ testLogFileName;
+ const auto filterstring = basePath.stem().string() + "-(\\d+)\\.log$";
+ const boost::regex my_filter(filterstring);
+
+ rolledFiles.clear();
+
+ boost::filesystem::directory_iterator end_itr;
+ for (boost::filesystem::directory_iterator i(
+ basePath.parent_path().string());
+ i != end_itr; ++i) {
+ if (boost::filesystem::is_regular_file(i->status())) {
+ std::string filename = i->path().filename().string();
+ boost::regex testPattern(filterstring);
+ boost::match_results<std::string::const_iterator> testMatches;
+ if (boost::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());
+ rolledFiles[index] = i->path();
+ }
+ }
+ }
+ }
+
+ void testLogFnError() {
+#ifdef WIN32
Review comment:
Is this because VC++ uses a different format for `__FUNCTION__`? It might be more appropriate to check for MSVC not WIN32 because if someone used Clang on Windows this might not hold true?
What does it look like if we switched to C++11 [`__func__`](https://en.cppreference.com/w/c/language/function_definition)?
----------------------------------------------------------------
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