[PATCH] D82844: [clangd] Set gRPC deadlines to all remote index requests

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:63
+      std::shared_ptr<grpc::Channel> Channel,
+      std::chrono::milliseconds DeadlineTime = std::chrono::milliseconds(500))
+      : Stub(remote::SymbolIndex::NewStub(Channel)),
----------------
I fear this may be too short - experience is going to be bad if it's really slow, but cutting off requests isn't going to be great.
I'd suggest bumping this up to at least a second... we should be sure we're logging timeouts too.

(If the connection is actually down, we shouldn't end up waiting for the deadline anyway in general: fail-fast is the default for grpc. https://github.com/grpc/grpc/blob/master/doc/wait-for-ready.md)


================
Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:95
+  // Each request will be terminated if it takes too long.
+  const std::chrono::milliseconds DeadlineWaitingTime;
 };
----------------
nit: I'd suggest generally avoiding const on non-pointer members, it messes with move semantics.

(In this case the object is always wrapped in unique_ptr before being exposed publicly, so it's actually fine. But have seen enough cases where it's initially fine and later not that I personally prefer to just always avoid it)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82844/new/

https://reviews.llvm.org/D82844