Add ability for control servers to broadcast to hubs via gRPC #40
Conversation
| return pb.NewHubServicesClient(cc), nil | ||
| } | ||
|
|
||
| g.mu.Lock() |
There was a problem hiding this comment.
Why do we need to use both RLock and Lock here in sequence? I guess this relates to your comment about there being a race condition -- does the race condition require we use a Lock on our second check instead of an RLock (is there a difference in behavior that contributes to a race condition), or is the reasoning "we need to do a Write, but first we have to check that Read didn't flake out on us previously, and that check has to be within the same Lock as the eventual write-action"?
There was a problem hiding this comment.
I think of it like a table lock in SQL. RLock locks all threads from reading, then sets the current target. Such that everyone behind this current Dial call has to wait to read until it is done. Only then can hypothetically the next person in line read the value, or not to see that it was changed by the person ahead of them. But this is in the weeds for me.
There was a problem hiding this comment.
Code looks good. As a disclaimer, I didn't dive too deep into fully understanding the logic, but I don't see anything that stands out to me. Generally speaking I feel like there could be more comments and in particular every exported function should probably be commented.
|
@mitchellh I'll take another pass at the comments. I've been working on having more of those docs on the first pass but missed some. |
|
This project in many ways is the last link I needed. This PR is another last link to keep that chain in a gRPC ring, thank you for your work here. |
|
|
These changes add a gRPC server within the hubs that is accessed from the control instances. The control instances are aware of the hubs and their addresses from consul, and use a internal authentication token to control access.