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

Fix Monitoring2Monitoring blob name generation, handle size mismatch when checking if blobs are synchronized #10123

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

joelverhagen
Copy link
Member

This PR fix two bugs I've found in dev based on SDK move (1 and 2, below), and one pre-existing bug found due to blob corruption in DEV (3, below)

Summary of changes:

  1. Scrub blob URL in logging, so we don't log SAS (query string)
  2. Handle mismatch of scheme between base URL (http) and blob item from list blobs (https)
    • This difference was leading to a substring being off by 1 ("http" is shorter than "https")
    • It would be a much larger change to make it so we never use "http" in memory, this behavior pre-existed in the SDK move (example)
  3. When blob sizes are different, do not consider them synchronized.
    • In DEV, there were some old blobs that had matching SHA-512 metadata but they were actually different. This was causing catalog2dnx to no-op. This can be easily avoided by also checking the file size. Of course this doesn't fix all problems if the SHA-512 is different, but it's an easy, safe improvement.

This was causing monitoring2monitoring to fail with an invalid blob name (double slashes).
Scrub blob URL for logging
@joelverhagen joelverhagen requested a review from a team as a code owner August 13, 2024 14:21
erdembayar
erdembayar previously approved these changes Aug 13, 2024
@joelverhagen joelverhagen merged commit 843a4eb into dev Aug 13, 2024
2 checks passed
@joelverhagen joelverhagen deleted the jver-monitoring branch August 13, 2024 22:43
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.

2 participants