Add retries to DatabaseWriter on partial success, and add unit tests #116
Conversation
| using System.Runtime.CompilerServices; | ||
|
|
||
| // Make internals visible to the test project | ||
| [assembly: InternalsVisibleTo("KustoSchemaTools.Tests")] |
There was a problem hiding this comment.
allows nice mockable unit tests
| @@ -10,17 +10,72 @@ namespace KustoSchemaTools.Parser.KustoWriter | |||
| { | |||
| public class DefaultDatabaseWriter : IDBEntityWriter | |||
| { | |||
| public async Task WriteAsync(Database sourceDb, Database targetDb, KustoClient client, ILogger logger) | |||
| public virtual async Task WriteAsync(Database sourceDb, Database targetDb, KustoClient client, ILogger logger) | |||
There was a problem hiding this comment.
this file needs substantial review.
At a high level, it breaks apart the WriteAsync call into testable methods: UpdatePrimary and UpdateFollowers.
I made it virtual to catch and count exceptions in the tests. Not required, but nice.
| logger.LogInformation($"Iteration {iterationCount}: Successfully applied {successes.Count} out of {results.Count} changes."); | ||
|
|
||
| // Decide whether to loop | ||
| keepGoing = successes.Count < results.Count && successes.Count > 0; |
There was a problem hiding this comment.
this should be: at least one failed but not all
There was a problem hiding this comment.
Pull Request Overview
This PR introduces retry logic to database schema operations to handle interdependent script deployment sequences. When multiple function scripts are changing simultaneously, sequencing problems can cause failures that can be resolved by retrying the operations until no more progress is made.
- Adds iterative retry logic that continues applying changes until no forward progress is achieved
- Refactors the
DefaultDatabaseWriterinto smaller, testable methods with internal visibility - Implements comprehensive unit tests to verify retry behavior and edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| KustoSchemaTools/Properties/AssemblyInfo.cs | Adds InternalsVisibleTo attribute to enable unit testing of internal methods |
| KustoSchemaTools/Parser/KustoWriter/DefaultDatabaseWriter.cs | Refactors WriteAsync method to implement retry logic and breaks down functionality into testable internal methods |
| KustoSchemaTools.Tests/DefaultDatabaseWriterTests.cs | Adds unit tests covering retry scenarios, success cases, and failure conditions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
|
||
| // Final status | ||
| Console.WriteLine($"Successfully applied {allResults.Count(r => r.Result != "Failed")} out of {allResults.Count} changes to {targetDb.Name}"); | ||
| foreach (var result in allResults) | ||
| { | ||
| Console.WriteLine($"{result.CommandType} ({result.OperationId}): {result.Result} => {result.Reason} ({result.CommandText})"); |
There was a problem hiding this comment.
Using Console.WriteLine directly in a class that accepts an ILogger parameter violates the dependency injection pattern. Consider using logger.LogInformation instead for consistency with the rest of the codebase.
| Console.WriteLine($"{result.CommandType} ({result.OperationId}): {result.Result} => {result.Reason} ({result.CommandText})"); | |
| logger.LogInformation($"Successfully applied {allResults.Count(r => r.Result != "Failed")} out of {allResults.Count} changes to {targetDb.Name}"); | |
| foreach (var result in allResults) | |
| { | |
| logger.LogInformation($"{result.CommandType} ({result.OperationId}): {result.Result} => {result.Reason} ({result.CommandText})"); |
Copilot uses AI. Check for mistakes.
| return allResults; | ||
| } | ||
|
|
||
| internal virtual async Task<List<ScriptExecuteCommandResult>> UpdateFollowers(Database sourceDb, ILogger logger) |
There was a problem hiding this comment.
I made this virtual async, but didn't actually add any tests for it yet.
| } | ||
|
|
||
| internal virtual List<IChange> GenerateChanges(Database targetDb, Database sourceDb, ILogger logger) | ||
| { | ||
| return DatabaseChanges.GenerateChanges(targetDb, sourceDb, targetDb.Name, logger); |
There was a problem hiding this comment.
this needs to be mockable, so move the static method call into a member method.
| { | ||
| batch.Add(sc); | ||
| continue; | ||
| // If we encounter an async script, execute any pending sync scripts first |
There was a problem hiding this comment.
I don't quite understand this logic, but I have added comments to describe what's happening.
| { | ||
| var batchResults = await ExecutePendingSync(databaseName, client, logger, pendingBatch); | ||
| results.AddRange(batchResults); | ||
| pendingBatch.Clear(); |
There was a problem hiding this comment.
OO, I think copilot added this clear() call.
That was not in the original, but it does seem to make sense, otherwise you'd execute them over and over.
There was a problem hiding this comment.
@whitleykeith this in particular needs review.
| } | ||
|
|
||
| private async Task<ScriptExecuteCommandResult> ExecuteAsyncCommand(string databaseName, KustoClient client, ILogger logger, DatabaseScriptContainer sc) | ||
| internal virtual async Task<ScriptExecuteCommandResult> ExecuteAsyncCommand(string databaseName, KustoClient client, ILogger logger, DatabaseScriptContainer sc) |
There was a problem hiding this comment.
I could technically reset this piece. I didn't change or add tests for it.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
zachariahcox
changed the title
When there are interdependent function scripts that are being changed at the same time, sometimes there will be a sequencing problem.
It may be that we can do work to get the script deployment sequence exactly perfect, but we can also try this.
In this PR, we just keep applying changes until they stop working 😂
This is a neat side effect of the way
.execute scriptworks in kusto when you allow it to continue on error.