rpc: make blockingQuery easier to read #12109
Conversation
To remove the TODO, and make it more readable. In general this reduces the scope of variables, making them easier to reason about. It also introduces more early returns so that we can see the flow from the structure of the function.
This safeguard should be safe to apply in general. We are already applying it to non-blocking queries that call blockingQuery, so it should be fine to apply it to others.
This helps keep the logic in blockingQuery more focused. In the future we may have a separate struct for RPC queries which may allow us to move this off of Server.
Remove some unnecessary comments around query_blocking metric. The only line that needs any comments in the atomic decrement. Cleanup the block and return comments and logic. The old comment about AbandonCh may have been relevant before, but it is expected behaviour now. The logic was simplified by inverting the err condition.
dnephin
added
theme/internal-cleanup
| if m.GetIndex() < 1 { | ||
| m.SetIndex(1) | ||
| } |
There was a problem hiding this comment.
Despite the implication by the old comment, this was done for all queries that went through blockingQuery , even those with index=0 that were not actually blocking queries.
With this change, we continue to set it for all queries that go through blockingQuery, but we will now also set the default response index in a few queries that do their own thing (ex: the Txn operation). That's why I had to modify the expected value of a couple tests below.
| // block until something changes, or the timeout | ||
| if err := ws.WatchCtx(ctx); err != nil { | ||
| // exit if we've reached the timeout, or other cancellation | ||
| return nil |
There was a problem hiding this comment.
I think there's a difference in codepath: previously,blockingQuery would return any non-nil err whereas here we suppress it and return nil instead.
If that's intentional, do we want a more robust error check here? It looks like we know what errors to expect but if ws.WatchCtx ever changes to return other kinds of errors this might lead to unexpected behavior
There was a problem hiding this comment.
I may be wrong, but I believe the old code would only ever return the err from fn(), which we still do here on line 970.
In the old code, the error from WatchCtx would only be scoped to that one block, and nothing in that block returned.
That's a good point about future compatibility though. I was also thinking of improving the godoc for WatchCtx so that it doesn't change functionality. I'd like to make the godoc for that method be more explicit about when an error can be returned (only if the context is cancelled), and what errors can be returned (only the context error).
I think if we did that, any change to errors returned by WatchCtx would be a breaking change. We could still guard against it here by checking the error, but I'm usually hesitant to add checks for conditions that can't currently exist.
There was a problem hiding this comment.
Ah good point, I missed the shadowed err from WatchCtx. I poked at the WatchCtx definition and what you said makes good sense! 👍
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/567779. |
This is being done to make space for fixing #9449. Best viewed by individual commit. There is more detail in the commit messages.
The current implementation of
blockingQueriesis difficult to read, and difficult to modify, for a few reasons.goto. While it is safe to usegotoin Go, most readers will not be used to following logic that uses goto. A for loop is not only much easier to reason about, it removes the need to pre-define a bunch of variables at the top of the function. Sometimes agotois the right solution for complicated logic, but I believe this refactor shows it is not necessary here, and that a for loop reads better.The getter methods on
QueryOptionsCompatandQueryMetaCompatalso make the code a little harder to read. It'd be great to use a struct field directly, or rename the interface methods to have them read a bit more naturally. Those renames are left for a future commit.