kbobyrev added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:114
assert(!ProjectRoot.empty());
+ ChannelStatusWatcher = std::thread([&Channel]() {
+ grpc_connectivity_state Status =
----------------
sammccall wrote:
> The thread is a bit surprising here. I was assuming we'd use some
> callback-based API to watch rather than spawn a thread to do the "work" of
> blocking on a change.
> Also this is going to "wake up" periodically to refresh the watch, which
> seems undesirable if we want to go truly idle.
>
> Ah, so there's no callback-type connection-changed API! Sorry to mislead you
> :-( I hadn't read the API carefully. (I think this is coming as some point -
> they're talking about replacing CompletionQueue with EventManager which is a
> callback-based abstraction)
>
> Seems the "async" option is to subscribe on a CompletionQueue, but then we
> have to poll it. We can use a zero timeout so the poll doesn't block... But
> then we might as well poll GetStatus directly.
>
> Then the question is where to do that from. What if we called GetStatus
> before/after each request, and reported whenever it has changed? That means:
>
> - no idle background work
> - we know the connection state for each request
> - we know when a request triggered a connection, and whether that connection
> succeeded in time for the request
> - if a connection goes idle, we don't know *when*, it gets reported on the
> next request
("resolved" in this version)
I can understand your point and I support the arguments but this would mean
that we only get status during requests and can't query it at arbitrary time
which seems like a loss. Maybe it's OK because there might not be many
interesting cases when we want to query the server status but not do any
requests and your proposal makes it easier to implement & maintain. But also
maybe it could be nice to have some kind of callback on status change.
As you mentioned, I was looking for something like this in gRPC itself but
couldn't find it there outside the CompletionQueue and EvenManager things.
Sadly, I don't think we'd be in a position to use the replacement even when
it's there since we have to support older gRPC versions :(
When I see the logs and the server connectivity updates are following the
requests, it looks OK but I'm not entirely sure giving up on _watching_ the
status is great. But it still makes life easier so maybe this change should be
about it.
================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:124
+ Status = Channel->GetState(/*try_to_connect=*/true);
+ log("Remote index connection status changed: {0}", Status);
+ }
----------------
sammccall wrote:
> Can we include the remote address and prior status?
>
> e.g. `"Remote index ({0}): {1} => {2}"`
I can see how it'd be nice but I'm not sure how to do this properly. My only
idea is to query prior status in the beginning of the request? Otherwise it
needs to be stored somewhere and gRPC interfaces require `const` functions, so
`IndexClient` does not change the state within requests. I could either wrap it
into something or have global `grpc_connnectivity_state` for the last observed
status but neither look OK. Maybe querying the prior status right before the
request is OK (that's what I've done here) but I'm not sure if that's exactly
what you suggested.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92198/new/
https://reviews.llvm.org/D92198
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
