kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/MonitoringService.proto:17
+ optional uint64 uptime = 1;
+ // Time since the index was built on the indexing machine.
+ optional uint64 freshness = 3;
----------------
mention the units in here too (e.g. seconds)
(it might be even better to just put that in the name actually, e.g.
uptime_seconds, as documentation for generated files might not show up in tools
like clangd :P, up to you though)
================
Comment at: clang-tools-extra/clangd/index/remote/MonitoringService.proto:18
+ // Time since the index was built on the indexing machine.
+ optional uint64 freshness = 3;
+ // ID of the indexed commit in Version Control System.
----------------
what happened to `2` ?
================
Comment at: clang-tools-extra/clangd/index/remote/MonitoringService.proto:18
+ // Time since the index was built on the indexing machine.
+ optional uint64 freshness = 3;
+ // ID of the indexed commit in Version Control System.
----------------
kadircet wrote:
> what happened to `2` ?
nit: maybe rename this to `index_age`
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:294
+
+ void updateIndex(const llvm::sys::TimePoint<> UpdateTime) {
+ IndexLastReload = UpdateTime;
----------------
nit: drop the `const`, as that's the style in clangd in general if we are going
to copy the parameters anyway.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:308
+ .count());
+ // FIXME(kirillbobyrev): This is not really "freshness", this is just the
+ // time since last index reload.
----------------
what's the issue to be fixed here exactly? do we want to display the index
build time instead of age? (if so what's the blocker)
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:317
+ const llvm::sys::TimePoint<> StartTime;
+ llvm::sys::TimePoint<> IndexLastReload;
+};
----------------
we should either use an atomic here or use a mutex. as the `updateIndex` and
serving RPCs are happening on different threads.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:344
Status->getLastModificationTime(), Status->getSize());
+ Monitor.updateIndex(Status->getLastModificationTime());
}
----------------
nit: i'd log after this one.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:457
- std::thread HotReloadThread([&Index, &Status, &FS]() {
+ Monitor Monitor(std::chrono::system_clock::now());
+
----------------
unless we call updateIndex here too, monitoring service won't know about the
one being served until the first reload right?
(another option would be to just not load the index at startup immediately, and
handle the initial load in HotReloadThread, but that can be a separate change.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98246/new/
https://reviews.llvm.org/D98246
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
