[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #918: GEODE-10016: Add map of threadId to threadName

[GitBox <gi...@apache.org>]

gaussianrecurrence commented on a change in pull request #918:
URL: https://github.com/apache/geode-native/pull/918#discussion_r799326466



##########
File path: cppcache/src/Log.cpp
##########
@@ -382,7 +383,8 @@ std::string Log::formatLogLine(LogLevel 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() << " "
+      << DistributedSystemImpl::getThreadName(std::this_thread::get_id())

Review comment:
       I am not totally sure about this, but changing the logging format might break the GNMSG tool that @pdxcodemonkey implemented some time ago. I am not sure what level of support you are intending to provide for this tool, but might be  good to make sure that the tool could handle both log formats

##########
File path: cppcache/src/ThreadPool.cpp
##########
@@ -16,6 +16,8 @@
  */
 #include "ThreadPool.hpp"
 
+#include <sstream>

Review comment:
       I think this might be a remaining of some dev debugging. Is this needed anymore?

##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -143,11 +147,24 @@ void DistributedSystemImpl::unregisterCliCallback(int appdomainId) {
   }
 }
 
-void DistributedSystemImpl::setThreadName(const std::string& threadName) {
+std::string DistributedSystemImpl::getThreadName(std::thread::id id) {

Review comment:
       Is there any particular reason why you would want to obtain the thread name from another thread which is not the caller thread? Because if there is no reason, I'd say it'd be best to get the thread ID of the current thread within the function, and that would make syntax a little bit simpler.

##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -143,11 +147,24 @@ void DistributedSystemImpl::unregisterCliCallback(int appdomainId) {
   }
 }
 
-void DistributedSystemImpl::setThreadName(const std::string& threadName) {
+std::string DistributedSystemImpl::getThreadName(std::thread::id id) {
+  std::string threadName = m_threadNames[id];
+  if (threadName == "") {
+    std::stringstream ss;
+    ss << id;
+    threadName = ss.str();
+  }
+  return threadName;
+}
+
+void DistributedSystemImpl::setThreadName(const std::string& threadName,

Review comment:
       Same comment regarding thread ID as for getThreadName, IMHO I don't see a reason why you would need to pass a thread ID here




-- 
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