Simplify the constructs we use to sync changes between client and OOP.#60761
Simplify the constructs we use to sync changes between client and OOP.#60761CyrusNajmabadi merged 11 commits intodotnet:mainfrom
Conversation
|
|
||
| using (Logger.LogBlock(FunctionId.SolutionChecksumUpdater_SynchronizePrimaryWorkspace, cancellationToken)) | ||
| { | ||
| var checksum = await solution.State.GetChecksumAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
pointless double computation. TryInvokeAsync already has to do this to get the solution checksum passed in the PinnedSolutionInfo that is passes into the lambda.
| private static async Task TestAssetAsync(object data) | ||
| { | ||
| var sessionId = 0; | ||
| var sessionId = Checksum.Create(ImmutableArray.CreateRange(Guid.NewGuid().ToByteArray())); |
There was a problem hiding this comment.
any dummy value woudl do. The mocks in these code actually end up not even using this.
|
|
||
| // verify initial setup | ||
| await workspace.ChangeSolutionAsync(solution); | ||
| solution = workspace.CurrentSolution; |
There was a problem hiding this comment.
prior code didn't represent how the workspace/oop actually work. We were forking the solution, but then telling the workspace to sync it's workspace (even though we hadn't applied the solution back to the workspace). This was working in the test code before because we were synthesizing a new workspace version every time we called UpdatePrimaryWorkspace. I changed that to actually pass the real value. But this means actually needing the real value to get updated. Hence the above lines.
| } | ||
|
|
||
| // make sure we always move remote workspace forward | ||
| private int _solutionVersion = 0; |
There was a problem hiding this comment.
this was an ugly test hack. I've removed it and updated the tests themselves to do the right thing.
| await client.TryInvokeAsync<IRemoteAssetSynchronizationService>( | ||
| solution, | ||
| async (service, solutionInfo, cancellationToken) => await service.SynchronizePrimaryWorkspaceAsync(solutionInfo, checksum, _solutionVersion++, cancellationToken), | ||
| async (service, solutionInfo, cancellationToken) => await service.SynchronizePrimaryWorkspaceAsync(solutionInfo, cancellationToken), |
There was a problem hiding this comment.
passing solutionInfo and checksum is entirely redundant (PinnedSolutionInfo has always contained the solution checksum).
| /// </summary> | ||
| [DataMember(Order = 0)] | ||
| public readonly int ScopeId; | ||
| public readonly Checksum SolutionChecksum; |
There was a problem hiding this comment.
note: we already had this. it just moved up as being the actual fully unique identifier.
| /// Map from solution checksum to its associated <see cref="SolutionState"/>. | ||
| /// </summary> | ||
| private readonly ConcurrentDictionary<int, (SolutionState Solution, SolutionReplicationContext ReplicationContext)> _solutionStates = new(concurrencyLevel: 2, capacity: 10); | ||
| private readonly ConcurrentDictionary<Checksum, (SolutionState Solution, SolutionReplicationContext ReplicationContext)> _solutionStates = new(concurrencyLevel: 2, capacity: 10); |
There was a problem hiding this comment.
note: i'm going to see about merging this dictionary with the one above.
|
@dibarbet this is ready for review now. |
| /// </summary> | ||
| [DataMember(Order = 0)] | ||
| public readonly int ScopeId; | ||
| public readonly Checksum SolutionChecksum; |
There was a problem hiding this comment.
iirc we had something like a serialization version or something - does that matter here? I don't think so since this is just OOP communication (which should always be using the same bits) but just wanted to confirm
Followup to #60758. Applies suggestion by @dibarbet to remove scope-id entirely and just use checksums to identify data.