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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
