pivotal-jbarrett commented on a change in pull request #918:
URL: https://github.com/apache/geode-native/pull/918#discussion_r801145569
##########
File path: cppcache/src/ExpiryTaskManager.cpp
##########
@@ -51,7 +51,7 @@ void ExpiryTaskManager::start() {
auto start_future = start_promise.get_future();
runner_ = std::thread{[this, &start_promise] {
start_promise.set_value(true);
- DistributedSystemImpl::setThreadName(NC_ETM_Thread);
+ Log::setThreadName(NC_ETM_Thread);
Review comment:
Can we just inline the string constants, they aren't used anywhere else.
##########
File path: cppcache/src/SystemProperties.cpp
##########
@@ -169,7 +169,8 @@ SystemProperties::SystemProperties(
m_sslTrustStore(DefaultSslTrustStore),
m_sslKeystorePassword(DefaultSslKeystorePassword),
m_conflateEvents(DefaultConflateEvents),
- m_threadPoolSize(DefaultThreadPoolSize),
+ // m_threadPoolSize(DefaultThreadPoolSize),
+ m_threadPoolSize(16),
Review comment:
Looks like an unintentional change!
##########
File path: cppcache/src/Log.cpp
##########
@@ -369,6 +372,46 @@ LogLevel Log::charsToLevel(const std::string& chars) {
}
}
+void Log::setThreadName(const std::string& threadName) {
Review comment:
Change signature to `std::string threadName` then `std::move` into
`g_threadName`.
##########
File path: cppcache/src/Log.cpp
##########
@@ -38,6 +38,7 @@
#include <geode/ExceptionTypes.hpp>
#include <geode/util/LogLevel.hpp>
+#include "DistributedSystemImpl.hpp"
Review comment:
You can drop this too I think.
##########
File path: cppcache/src/Log.cpp
##########
@@ -378,12 +421,21 @@ std::string Log::formatLogLine(LogLevel level) {
now - std::chrono::system_clock::from_time_t(secs));
auto tm_val = apache::geode::util::chrono::localtime(secs);
+ std::thread::id id = std::this_thread::get_id();
Review comment:
Why the variable?
##########
File path: cppcache/src/Log.cpp
##########
@@ -60,6 +61,8 @@ static FILE* g_log = nullptr;
static std::string g_hostName;
+static thread_local std::string g_threadName;
Review comment:
See https://google.github.io/styleguide/cppguide.html#thread_local and
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
for naming guidelines. I think you want `kThreadName` but I am not too keen on
that given it isn't really a constant.
##########
File path: cppcache/src/DistributedSystemImpl.hpp
##########
@@ -24,6 +24,7 @@
#include <memory>
#include <mutex>
#include <string>
+#include <thread>
Review comment:
Drop this.
##########
File path: cppcache/src/Log.cpp
##########
@@ -378,12 +421,21 @@ std::string Log::formatLogLine(LogLevel level) {
now - std::chrono::system_clock::from_time_t(secs));
auto tm_val = apache::geode::util::chrono::localtime(secs);
+ std::thread::id id = std::this_thread::get_id();
+ std::stringstream ss;
+ ss << id;
+ std::string threadIdWithName;
+ if (getThreadName() != "") {
+ threadIdWithName = ss.str() + " (" + getThreadName() + ")";
+ } else {
+ threadIdWithName = ss.str();
+ }
+
msg << "[" << Log::levelToChars(level) << " "
<< std::put_time(&tm_val, "%Y/%m/%d %H:%M:%S") << '.' <<
std::setfill('0')
<< std::setw(6) << microseconds.count() << ' '
<< std::put_time(&tm_val, "%z ") << g_hostName << ":"
- << boost::this_process::get_id() << " " << std::this_thread::get_id()
- << "] ";
+ << boost::this_process::get_id() << " " << threadIdWithName << "] ";
Review comment:
just
```c++
.. << std::this_thread::id << g_threadName << ...
```
Avoid the extra stream creation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
