[PATCH] D92198: [clangd] Log remote index connectivity status

[Kirill Bobyrev via Phabricator via cfe-commits]

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