[GitHub] [trafficserver] jrushford commented on a change in pull request #8585: Clean up of next hop HostRecord class.

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

jrushford commented on a change in pull request #8585:
URL: https://github.com/apache/trafficserver/pull/8585#discussion_r780490418



##########
File path: include/tscore/ConsistentHash.h
##########
@@ -32,8 +32,8 @@
  */
 
 struct ATSConsistentHashNode {
-  std::atomic<bool> available;
-  char *name;
+  std::atomic<bool> available{false};

Review comment:
       I think we should initialize available to true.  All newly created host's should be available

##########
File path: proxy/http/remap/NextHopSelectionStrategy.h
##########
@@ -103,64 +103,27 @@ struct NHProtocol {
   std::string health_check_url;
 };
 
-struct HostRecord : ATSConsistentHashNode {
-  std::mutex _mutex;
+struct HostRecordCfg {
   std::string hostname;
-  std::atomic<time_t> failedAt;
-  std::atomic<uint32_t> failCount;
-  std::atomic<time_t> upAt;
-  float weight;
-  std::string hash_string;
-  int host_index;
-  int group_index;
-  bool self = false;
   std::vector<std::shared_ptr<NHProtocol>> protocols;
+  float weight{0};
+  std::string hash_string;
+};
 
-  // construct without locking the _mutex.
-  HostRecord()
-  {
-    hostname    = "";
-    failedAt    = 0;
-    failCount   = 0;
-    upAt        = 0;
-    weight      = 0;
-    hash_string = "";
-    host_index  = -1;
-    group_index = -1;
-    available   = true;
-  }
-
-  // copy constructor to avoid copying the _mutex.
-  HostRecord(const HostRecord &o)
-  {
-    hostname    = o.hostname;
-    failedAt    = o.failedAt.load();
-    failCount   = o.failCount.load();
-    upAt        = o.upAt.load();
-    weight      = o.weight;
-    hash_string = o.hash_string;
-    host_index  = -1;
-    group_index = -1;
-    available   = true;
-    protocols   = o.protocols;
-  }
-
-  // assign without copying the _mutex.
-  HostRecord &
-  operator=(const HostRecord &o)
-  {
-    hostname    = o.hostname;
-    failedAt    = o.failedAt.load();
-    failCount   = o.failCount.load();
-    upAt        = o.upAt.load();
-    weight      = o.weight;
-    hash_string = o.hash_string;
-    host_index  = o.host_index;
-    group_index = o.group_index;
-    available   = o.available.load();
-    protocols   = o.protocols;
-    return *this;
-  }
+struct HostRecord : public ATSConsistentHashNode, public HostRecordCfg {
+  std::mutex _mutex;

Review comment:
       @ywkaras I originally defined the constructor, copy constructor and assignment operator to avoid copying the state of the mutex in the POD.  It's my understanding that this should be done, am I wrong about that?




-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org