Skip to content

Clean up of next hop HostRecord class. #8585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ywkaras merged 1 commit into from
Jan 11, 2022
Merged

Clean up of next hop HostRecord class. #8585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/tscore/ConsistentHash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
*/

struct ATSConsistentHashNode {
std::atomic<bool> available;
char *name;
std::atomic<bool> available{true};
char *name{nullptr};
};

std::ostream &operator<<(std::ostream &os, ATSConsistentHashNode &thing);
Expand Down
6 changes: 3 additions & 3 deletions proxy/http/remap/NextHopSelectionStrategy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ NextHopSelectionStrategy::Init(ts::Yaml::Map &n)
std::vector<std::shared_ptr<HostRecord>> hosts_inner;

for (unsigned int hst = 0; hst < hosts_list.size(); ++hst) {
std::shared_ptr<HostRecord> host_rec = std::make_shared<HostRecord>(hosts_list[hst].as<HostRecord>());
std::shared_ptr<HostRecord> host_rec = std::make_shared<HostRecord>(hosts_list[hst].as<HostRecordCfg>());
host_rec->group_index = grp;
host_rec->host_index = hst;
if ((self_host == host_rec->hostname) || mach->is_self(host_rec->hostname.c_str())) {
Expand Down Expand Up @@ -311,9 +311,9 @@ NextHopSelectionStrategy::responseIsRetryable(int64_t sm_id, HttpTransact::Curre

namespace YAML
{
template <> struct convert<HostRecord> {
template <> struct convert<HostRecordCfg> {
static bool
decode(const Node &node, HostRecord &nh)
decode(const Node &node, HostRecordCfg &nh)
{
ts::Yaml::Map map{node};
ts::Yaml::Map *mmap{&map};
Expand Down
75 changes: 20 additions & 55 deletions proxy/http/remap/NextHopSelectionStrategy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

#pragma once

#include <utility>

#include "ts/parentselectdefs.h"
#include "ParentSelection.h"
#include "HttpTransact.h"
Expand Down Expand Up @@ -103,64 +105,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;
Copy link
Contributor

@jrushford jrushford Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jrushford jrushford Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see. never mind, you added = delete

Copy link
Contributor

@jrushford jrushford Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine now, I didn't notice you deleted the operator = and copy constructor. This make for less maintenance if someone adds a field.

std::atomic<time_t> failedAt{0};
std::atomic<uint32_t> failCount{0};
std::atomic<time_t> upAt{0};
int host_index{-1};
int group_index{-1};
bool self{false};

explicit HostRecord(HostRecordCfg &&o) : HostRecordCfg(std::move(o)) {}

// No copying or moving.
HostRecord(const HostRecord &) = delete;
HostRecord &operator=(const HostRecord &) = delete;

// locks the record when marking this host down.
void
Expand Down