Skip to content
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

Merged
merged 26 commits into from
Dec 15, 2023
Merged

[vcr-2.0] Log corrupt BlobIDs to Azure Table #2675

merged 26 commits into from
Dec 15, 2023

Conversation

snalli
Copy link
Contributor

@snalli snalli commented Dec 14, 2023

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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (8ca2a0e) 72.53% compared to head (43a074d) 72.61%.

Files Patch % Lines
...b/ambry/cloud/azure/AzureCloudDestinationSync.java 58.62% 10 Missing and 2 partials ⚠️
...va/com/github/ambry/cloud/azure/StorageClient.java 55.00% 7 Missing and 2 partials ⚠️
.../java/com/github/ambry/cloud/VcrReplicaThread.java 42.85% 4 Missing ⚠️
.../java/com/github/ambry/cloud/CloudDestination.java 50.00% 1 Missing ⚠️
...loud/azure/ConnectionStringBasedStorageClient.java 88.88% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@snalli snalli changed the title create table entity [vcr-2.0] Log corrupted BlobIDs to Azure Table Dec 14, 2023
@snalli snalli changed the title [vcr-2.0] Log corrupted BlobIDs to Azure Table [vcr-2.0] Log corrupt BlobIDs to Azure Table Dec 14, 2023
Copy link
Collaborator

@JingQianCloud JingQianCloud left a 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)
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

@snalli snalli merged commit 2c3419c into master Dec 15, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants