[
https://issues.apache.org/jira/browse/GEODE-8806?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17262750#comment-17262750
]
ASF GitHub Bot commented on GEODE-8806:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #718:
URL: https://github.com/apache/geode-native/pull/718#discussion_r555152211
##########
File path: cppcache/integration/test/CacheXmlTest.cpp
##########
@@ -44,8 +44,7 @@ apache::geode::client::Cache createCacheUsingXmlConfig(
CacheFactory cacheFactory;
- auto cache = cacheFactory.set("log-level", "debug")
- .set("log-file", "geode_native.log")
+ auto cache = cacheFactory.set("log-level", "none")
.set("statistic-sampling-enabled", "false")
.set("cache-xml-file", xmlFile.c_str())
Review comment:
Not your fault, but if there is anything else to clean up can you figure
out why this is producing a `c_str()` only to covert it back to a `std::string`.
##########
File path: cppcache/src/Log.cpp
##########
@@ -502,178 +351,77 @@ LogLevel Log::charsToLevel(const std::string& chars) {
}
}
-char* Log::formatLogLine(char* buf, LogLevel level) {
- if (g_pid == 0) {
- g_pid = boost::this_process::get_id();
- ACE_OS::uname(&g_uname);
- }
+std::string Log::formatLogLine(LogLevel level) {
+ std::stringstream msg;
const size_t MINBUFSIZE = 128;
auto now = std::chrono::system_clock::now();
auto secs = std::chrono::system_clock::to_time_t(now);
auto microseconds = std::chrono::duration_cast<std::chrono::microseconds>(
now - std::chrono::system_clock::from_time_t(secs));
auto tm_val = apache::geode::util::chrono::localtime(secs);
- auto pbuf = buf;
- pbuf += std::snprintf(pbuf, 15, "[%s ", Log::levelToChars(level));
- pbuf += std::strftime(pbuf, MINBUFSIZE, "%Y/%m/%d %H:%M:%S", &tm_val);
- pbuf += std::snprintf(pbuf, 15, ".%06" PRId64 " ",
- static_cast<int64_t>(microseconds.count()));
- pbuf += std::strftime(pbuf, MINBUFSIZE, "%Z ", &tm_val);
+ msg << "[" << Log::levelToChars(level) << " ";
+ char timebuf[MINBUFSIZE];
- std::snprintf(pbuf, 300, "%s:%d %" PRIu64 "] ", g_uname.nodename, g_pid,
- hacks::aceThreadId(ACE_OS::thr_self()));
+ std::strftime(timebuf, MINBUFSIZE, "%Y/%m/%d %H:%M:%S", &tm_val);
+ msg << timebuf;
- return buf;
-}
+ std::snprintf(timebuf, 15, ".%06" PRId64 " ",
+ static_cast<int64_t>(microseconds.count()));
+ msg << timebuf << " ";
-void Log::put(LogLevel level, const std::string& msg) {
- put(level, msg.c_str());
+ std::strftime(timebuf, MINBUFSIZE, "%Z ", &tm_val);
+ msg << timebuf << " " << boost::asio::ip::host_name() << ":"
Review comment:
I am a bit concerned this may result in a system call the resolver for
every log message. The previous code cached this. I suggest a `::hostname()`
method with a static resolved hostname using this method (Meyer's Singleton).
The likely hood of the hostname changing over the life of a client, while not
0, is pretty darn close to 0.
##########
File path: cppcache/src/Log.cpp
##########
@@ -502,178 +351,77 @@ LogLevel Log::charsToLevel(const std::string& chars) {
}
}
-char* Log::formatLogLine(char* buf, LogLevel level) {
- if (g_pid == 0) {
- g_pid = boost::this_process::get_id();
- ACE_OS::uname(&g_uname);
- }
+std::string Log::formatLogLine(LogLevel level) {
+ std::stringstream msg;
const size_t MINBUFSIZE = 128;
auto now = std::chrono::system_clock::now();
auto secs = std::chrono::system_clock::to_time_t(now);
auto microseconds = std::chrono::duration_cast<std::chrono::microseconds>(
now - std::chrono::system_clock::from_time_t(secs));
auto tm_val = apache::geode::util::chrono::localtime(secs);
- auto pbuf = buf;
- pbuf += std::snprintf(pbuf, 15, "[%s ", Log::levelToChars(level));
- pbuf += std::strftime(pbuf, MINBUFSIZE, "%Y/%m/%d %H:%M:%S", &tm_val);
- pbuf += std::snprintf(pbuf, 15, ".%06" PRId64 " ",
- static_cast<int64_t>(microseconds.count()));
- pbuf += std::strftime(pbuf, MINBUFSIZE, "%Z ", &tm_val);
+ msg << "[" << Log::levelToChars(level) << " ";
+ char timebuf[MINBUFSIZE];
- std::snprintf(pbuf, 300, "%s:%d %" PRIu64 "] ", g_uname.nodename, g_pid,
- hacks::aceThreadId(ACE_OS::thr_self()));
+ std::strftime(timebuf, MINBUFSIZE, "%Y/%m/%d %H:%M:%S", &tm_val);
+ msg << timebuf;
- return buf;
-}
+ std::snprintf(timebuf, 15, ".%06" PRId64 " ",
Review comment:
You should be able to do with with std stream formatting directives and
avoid the extra calls to `snprintf`.
----------------------------------------------------------------
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:
us...@infra.apache.org
> Native client does not honor log-file-size-limit properly
> ---------------------------------------------------------
>
> Key: GEODE-8806
> URL: https://issues.apache.org/jira/browse/GEODE-8806
> Project: Geode
> Issue Type: Bug
> Components: native client
> Reporter: Blake Bender
> Priority: Major
> Labels: pull-request-available
>
> As a user, I need to be able to limit the size of my log files and the amount
> of disk space used by geode-native. Unfortunately, the 10.x native client
> doesn't appear to "roll" log files correctly.
>
> repro steps:
> i. In a client application, set `log-file-size-limit` to 1 (for 1MB),
> `log-level` to `debug`, and `log-file` to something convenient like
> `native_app.log`
> ii. Run a long operation in the client that will generate a huge amount of
> logging, like 100,000 puts to a region
>
> expected behavior:
> * when native_app.log grows to > 1MB in size, it is followed by
> native_app-1.log, native_app-2.log, etc, each of which is ~1MB in size
> actual behavior:
> * native_app.log grows to > 1MB, then geode-native stops logging
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
