[PATCH] D98246: [clangd] Add basic monitoring info request for remote index server
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 10 00:51:57 PST 2021
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:16
+message MonitoringInfoReply { optional string info = 1; }
+
----------------
sammccall wrote:
> kadircet wrote:
> > i am not sure if having more structure here immediately vs. incrementally makes much difference, we should at least settle on the proto initially, and can fill in those fields in incremental steps if need be (but i don't expect those changes to be huge, so it should be fine to just cram them into this patch).
> >
> > i think we need more structure here, rather than just string. for starters:
> > - an unsigned for uptime (probably in seconds)
> > - another unsgined (or timepoint object) for indicating the build time of currently used index
> >
> > i am not sure if we got anything else we can dispatch immediately, but can probably incorporate things like QPS and more details about the loaded index if turns out to be useful/needed.
> it seems unlikely to me that `string info` is the right format, but I can't tell whether this is a placeholder or not
> we should at least settle on the proto initially, and can fill in those fields in incremental steps
Agree, incremental development is nice, but the scope/responsibility of RPCs is fairly important and not that easy to change later.
(Sorry, I hadn't seen this review wasn't actually assigned to me, happy to leave this with you!)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98246/new/
https://reviews.llvm.org/D98246
More information about the cfe-commits
mailing list