On 12/2/20 8:48 AM, Thayne McCombs wrote:
Thanks for your feedback. I have some followup questions.
I think you should initialized this ebtree with EB_ROOT_UNIQUE as
value.
I'm not sure I understand what the distinction between EB_ROOT and
EB_ROOT_UNIQUE is. I'm happy to add this, but I'd like to understand
why it should be EB_ROOT_UNIQUE here (since it looks like it is
initialized as EB_ROOT for used_server_id and used_listener_id, and I
can't find where it is initialized for used_server_name, which I
believe would be equivalent since EB_ROOT is basically just zero
initialized).
EB_NORMAL ebtree can store entries with the same key contrary to
EB_UNIQUE ebtre (see include/import/ebtree.h for the details.).
You are true about EB_ROOT initialization which is a zero initialization
which is not necessary if the proxies are calloc()'ed. EB_ROOT_UNIQUE is
not a zero initialization.
I think this is dangerous to initialize two objects with the same value
as a string. I would strdup() <key> to initialize s->addr_node.key.
ok. I was following the pattern used with conf.name in struct server.
Would it be better to use strdup, or to not have a separate field for
addr_desc, and just use addr_node.key (which is a void*)? If the
latter maybe rename addr_node as addr_desc?
Yes, this is the question I forgot to ask: if addr_desc field is not
necessary, remove it. addr_node is ok for the name.
Additionally:
In srv_set_addr_desc, would it be better to hold the lock over the
entire operation, or just while we are actually manipulating the tree?
Meaning should I release the lock between deleting the old node and
inserting the new node? And should this use the existing proxy lock,
or should there be a separate dedicated lock for used_server_addr?
You must add a comment for this function about the lock for the server.
The lock for the server must be hold. But at parsing time this is not
necessary, so you do not have to lock the server.
Then you must lock the proxy to manipulate the ebtree attached to the
proxy when you enter srv_set_addr_desc() , and release before leaving it.
I'm also a little bit concerned about leaking memory in the
server_key_dict. I couldn't find anything that cleans up these keys,
although there is a refcount on them. In an environment that uses
service discovery and the servers change frequently, this could lead
to a memory leak as this dict only grows in size.
The server names were not supposed to change. If the server addresses
changes, we must remove the entries from the dictionary if they are no
more used. I guess there may be several servers with the same address.
