kadircet added inline comments.
================
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.
----------------
kbobyrev wrote:
> kadircet wrote:
> > 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)
> The point here is that is the index load freshness but it's not the index
> load, not the index age itself. The index age is e.g. the time of the last
> commit being indexed. Hence, we don't actually have this information _right
> now_ - it should be provided separately (probably in a way of adjacent
> `index.metadata` file or something similar). E.g. when we download from
> GitHub release, we do know the time an index was uploaded, that's a better
> estimate of the index "age" probably. But what the comment in Proto file says
> is actually more like the commit time, e.g. when did the last change that
> made it into the index happen.
ah i see, yes you are totally right.
maybe rephrase as `We are currently making use of the last modification time of
the index artifact to deduce its age. This is wrong as it doesn't account for
the indexing delay. Propagate some metadata with the index artifacts to
indicate time of the commit we indexed.`
================
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());
+
----------------
kbobyrev wrote:
> kadircet wrote:
> > 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.)
> I think using `Status.getLastModificationTime()` should be correct, right?
> This is the index we actually loaded (checked above).
right that would do, but now we are lying about the server's uptime :D
so we should explicitly call
`Monitor.updateIndex(Status->getLastModificationTime());` and still initialize
monitoring service with `system_clock::now`.
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
