-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[vcr-2.0] Log corrupt BlobIDs to Azure Table #2675
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2675 +/- ##
============================================
+ Coverage 72.53% 72.61% +0.08%
- Complexity 11552 11574 +22
============================================
Files 810 809 -1
Lines 65687 65755 +68
Branches 8025 8030 +5
============================================
+ Hits 47644 47747 +103
+ Misses 15427 15394 -33
+ Partials 2616 2614 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good to me. one comment left.
@Override | ||
protected TableServiceClient buildTableServiceClient(HttpClient httpClient, Configuration configuration, | ||
RetryOptions retryOptions, AzureCloudConfig azureCloudConfig) { | ||
return new TableServiceClientBuilder().connectionString(azureCloudConfig.azureTableConnectionString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, azureTableConnectionString is null.
Looks like getTableServiceClient will throw exception to the constructor.
Do you test the case when azureTableConnectionString is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a method validateTableServiceConfigs
which will fail VCR startup if the table-conn-str is null and we are using a conn-str client. The conn-str varies depending on the azure account, so we cannot hardcode it here.
This patch adds support to record corrupted blobIDs in Azure Table. It extends ReplicaThread to create VcrReplicaThread which overrides logToExternalTable, such that replication between servers is unaffected.