kadircet added a comment.
thanks! this mostly LG, just a couple comments here and there.
> The current format is "store seconds since X event".
I think this is great for certain things like freshness, so thanks!
> Should we store UNIX timestamps instead? This is probably not portable until
> C++20, so maybe it isn't a great idea.
But I believe we also need to record timepoints. what about just storing
seconds since unix epoch in an int64?
i don't follow why it would be unportable, as by definition it is the time
elapsed since 1-1-1970, so i don't think it has anything specific to UNIX
itself, apart from the name.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:27
+message MonitoringInfoRequest {}
+message MonitoringInfoReply {
----------------
nit: i'd prefer these to be on a separate file (both messages and the service
definition), WDYT?
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:32
+ // Time since the last index reload (in seconds).
+ optional uint64 time_since_reload = 2;
+ // FIXME(kirillbobyrev): The following fields should be provided with the
----------------
this and freshness has very little difference. i'd either drop one (preferably
this one), or change this one to be a timepoint for the latest index reload.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:33
+ optional uint64 time_since_reload = 2;
+ // FIXME(kirillbobyrev): The following fields should be provided with the
+ // index (probably in adjacent metadata.txt file next to loaded .idx).
----------------
i think this fixme belongs to the service implementation.
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:37
+ optional uint64 freshness = 3;
+ // Time since the index was built on the indexing machine.
+ optional string index_commit_hash = 4;
----------------
i smell some c/p
================
Comment at: clang-tools-extra/clangd/index/remote/Service.proto:41
+ optional string index_link = 5;
+ optional string metadata = 9000;
+}
----------------
what's the point of this exactly?
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:351
IdleTimeoutSeconds * 1000);
Builder.RegisterService(&Service);
std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
----------------
don't we need to register monitoring service in here?
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
