Implemented distributed cache over LLS. #1200
Conversation
| /// <returns> | ||
| /// Result providing the call's completion status. True if the replacement was completed successfully, false otherwise. | ||
| /// </returns> | ||
| public static Possible<bool> CompareExchange( |
There was a problem hiding this comment.
Do we want to start using 'Possible' types from BuildXL.Utilities?
I think it is a bit confusing to introduce yet another result type here...
There was a problem hiding this comment.
It seems that the type was introduced already... But the question still holds...
In reply to: 345368562 [](ancestors = 345368562)
There was a problem hiding this comment.
just curious, why not to use BoolResult here instead?
In reply to: 345368790 [](ancestors = 345368790,345368562)
There was a problem hiding this comment.
This is code I originally wrote a few months ago. Used CompareExchange because I didn't know how to convert to BoolResult
In reply to: 345369015 [](ancestors = 345369015,345368790,345368562)
| default: | ||
| throw new ArgumentOutOfRangeException($"Unknown event kind '{kind}'."); | ||
| } | ||
| } | ||
|
|
||
| /// <nodoc /> | ||
| public void Serialize(BuildXLWriter writer) | ||
| public virtual void Serialize(BuildXLWriter writer) |
There was a problem hiding this comment.
Minor, but curious why you decided to use another pattern here?
Before the full dispatch was happened here, and now it is virtual. I think it make sense to stay consistent unless there is a reason to follow different pattern in different cases.
| /// </summary> | ||
| public bool TryGetLocalLocationStore(out LocalLocationStore localLocationStore) | ||
| { | ||
| if (_contentLocationStore is TransitioningContentLocationStore tcs |
There was a problem hiding this comment.
It is interesting that TryGetLocalLocationStore is quite different from LocalLocationStore property.
One may think that these two are the same and one is implemented on top of another. But only the 'TryGet' one will return false in case if the store is disabled.
| @@ -15,13 +15,13 @@ internal class RedisMemoizationStore : DatabaseMemoizationStore | |||
| { | |||
| /// <nodoc /> | |||
| private RedisMemoizationStore(ILogger logger, IClock clock, RedisDatabaseAdapter redis, TimeSpan memoizationExpiryTime) | |||
There was a problem hiding this comment.
ILogger logger [](start = 38, length = 14)
Logger is not being used any more
| @@ -15,13 +15,13 @@ internal class RedisMemoizationStore : DatabaseMemoizationStore | |||
| { | |||
| /// <nodoc /> | |||
| private RedisMemoizationStore(ILogger logger, IClock clock, RedisDatabaseAdapter redis, TimeSpan memoizationExpiryTime) | |||
| : base(logger, new RedisMemoizationDatabase(redis, clock, memoizationExpiryTime)) | |||
| : base(new RedisMemoizationDatabase(redis, clock, memoizationExpiryTime)) | |||
| { | |||
| } | |||
|
|
|||
| /// <nodoc /> | |||
| public RedisMemoizationStore(ILogger logger, RedisMemoizationDatabase database) | |||
There was a problem hiding this comment.
ILogger logger [](start = 37, length = 14)
Same here
| @@ -42,7 +42,7 @@ public class DistributedCacheSessionTracer : MemoizationStoreTracer, IMetadataCa | |||
| /// Initializes a new instance of the <see cref="DistributedCacheSessionTracer" /> class. | |||
| /// </summary> | |||
| public DistributedCacheSessionTracer(ILogger logger, string name) | |||
There was a problem hiding this comment.
ILogger logger, [](start = 45, length = 15)
Logger not used anymore
| @@ -34,7 +34,7 @@ public class SQLiteMemoizationStoreTracer : MemoizationStoreTracer, ISQLiteDatab | |||
| /// <param name="logger">Logger</param> | |||
| /// <param name="name">Tracer Name</param> | |||
| public SQLiteMemoizationStoreTracer(ILogger logger, string name) | |||
There was a problem hiding this comment.
ILogger logger, [](start = 44, length = 15)
Logger not used anymore
|
|
||
| // Update the entry if the current entry is newer | ||
| // TODO: Use real versioning scheme for updates to resolve possible race conditions and | ||
| // issues with time comparison due to clock skew |
There was a problem hiding this comment.
Comment explaining why this isn't a frequent enough occurrence so as to care?
| var redisStore = (RedisGlobalStore)localLocationStore.GlobalStore; | ||
|
|
||
| MemoizationStore = new DatabaseMemoizationStore(new DistributedMemoizationDatabase( | ||
| localLocationStore, |
There was a problem hiding this comment.
When ContentLocationDatabase is created without MetadataGarbageCollectionEnabled=true, metadata GC won't run. As things are right now, the CLDB instance won't perform any metadata GC.
No description provided.