Clean up of next hop HostRecord class. #8585
Conversation
|
I have very mixed feelings about this. An ideal solution isn't apparent. Every option has pros and a lot of cons.
I'm a big fan of deleting the copy and move - HostRecord conceptually should never be copied after initialization.
Not such a big fan of the multiple inheritance. I think the pains of multiple inheritance go without saying. But the multiple inheritance can easily be avoided by using composition instead. I see the complexity as the bigger question. Bigger picture, this adds another layer of abstraction, for a very small object. It's an awful lot of complexity for what really ought to be a dead-simple POD class. Is it really worth all this complexity, the added readability and maintainability costs, to reduce the chance of one small type of bug? I honestly don't know. |
include/tscore/ConsistentHash.h
Outdated
| struct ATSConsistentHashNode { | ||
| std::atomic<bool> available; | ||
| char *name; | ||
| std::atomic<bool> available{false}; |
There was a problem hiding this comment.
I think we should initialize available to true. All newly created host's should be available
| return *this; | ||
| } | ||
| struct HostRecord : public ATSConsistentHashNode, public HostRecordCfg { | ||
| std::mutex _mutex; |
There was a problem hiding this 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?
There was a problem hiding this comment.
oh, I see. never mind, you added = delete
There was a problem hiding this comment.
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.
|
I guess I was figuring that, since the PR makes the code more than 30 lines shorter, it was making it less complicated. But maybe I just think too simplistically since I didn't go to graduate school. |
I don't agree that lines of code is a strong indicator of complexity, or difficulty to read and maintain. Thirty lines fewer of assembly would not be less complex, but 30 lines more of Go probably would be. I do see an additional layer of abstraction, another class, and multiple inheritance as more complex. But as I said, I have mixed feelings, and I do agree there are advantages. I won't object if @jrushford or anyone else wants to approve. |
ywkaras
force-pushed
the
NHHostRecCleanup
branch
from
dfcd4ce to
b72ecfb
Compare
|
It's accurate to observe that I'm using inheritance for something that is not intuitively an is-a relationship. I could instead have a HostRecordCfg substructure, instead of a base class, but that would mean more changes to the existing code. But it's fairly common to use inheritance in "tricky" ways in C++: https://stackoverflow.com/questions/4041447/how-is-stdtuple-implemented |
ywkaras
requested review from
jrushford
and removed request for
SolidWallOfCode and
bryancall
(cherry picked from commit 71e4aba)
|
Cherry-picked to v9.2.x |
* asf/9.2.x: Updated ChangeLog TSHttpTxnCacheLookupStatusGet: handle cannot respond cases (apache#8545) Update to Proxy Verifier version v2.3.0 (apache#8608) Don't use Http1ClientTransaction as an event handler (apache#8609) Eliminate erroneous self-loop error on transparent mode (apache#8586) Clean up of next hop HostRecord class. (apache#8585) Propagate accept options to HTTP/2 (apache#8594) add --with-mimalloc option (apache#8233) Fix transparent mode documentation (apache#8593) Docs: Slack instead of irc (apache#8599) LogFilter: fix NULL termination check (apache#8603) Fixes a scoping bug that leads to "sticky" weights (apache#8606)
4 participants
No description provided.