feat(sync): add ETag and content hash support to blob sync#1991
feat(sync): add ETag and content hash support to blob sync#1991leakonvalinka wants to merge 6 commits into
Conversation
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughBlob synchronization now compares object attributes and canonicalized JSON body hashes before publishing, persists ETag and body-hash state, synchronizes readiness access, centralizes hashing for blob and HTTP sync, replaces the test mock driver, and expands change-detection and resync tests. ChangesBlob Sync Change Detection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Poller
participant BlobSync
participant BlobBucket
participant DataChannel
Poller->>BlobSync: sync()
BlobSync->>BlobBucket: fetchObjectAttributes()
BlobBucket-->>BlobSync: ETag and ModTime
alt attributes changed
BlobSync->>BlobBucket: fetchObject()
BlobBucket-->>BlobSync: converted JSON and body hash
alt body hash changed
BlobSync->>DataChannel: publish sync.DataSync
else body hash unchanged
BlobSync-->>Poller: skip publication
end
else attributes unchanged
BlobSync-->>Poller: skip full-object fetch
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
dosubot
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/pkg/sync/blob/blob_sync.go (1)
183-186: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider hashing the converted JSON rather than raw bytes.
generateShahashes the rawdata(pre-conversion bytes), butfetchObjectreturns the convertedjsonstring for publishing. If a YAML file is reformatted without changing the semantic flag configuration, the raw bytes change (different hash) while the JSON output stays the same — triggering an unnecessary fetch+publish of identical content.Hashing the converted JSON instead would catch this case and align the change-detection hash with what is actually published.
♻️ Proposed refactor: hash the converted JSON
func (hs *Sync) fetchObject(ctx context.Context, bucket *blob.Bucket) (string, string, error) { r, err := bucket.NewReader(ctx, hs.Object, nil) if err != nil { return "", "", fmt.Errorf("error opening reader for object %s/%s: %w", hs.Bucket, hs.Object, err) } defer r.Close() data, err := io.ReadAll(r) if err != nil { return "", "", fmt.Errorf("error downloading object %s/%s: %w", hs.Bucket, hs.Object, err) } json, err := utils.ConvertToJSON(data, filepath.Ext(hs.Object), r.ContentType()) if err != nil { return "", "", fmt.Errorf("error converting blob data to json: %w", err) } - return json, hs.generateSha(data), nil + return json, hs.generateSha([]byte(json)), nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/sync/blob/blob_sync.go` around lines 183 - 186, The change-detection hash in generateSha is based on the raw input bytes, but fetchObject publishes the converted JSON, so equivalent YAML reformatting can trigger unnecessary updates. Update the hashing flow around generateSha/fetchObject so the hash is computed from the converted JSON string (the same payload that gets published), and ensure the comparison uses that canonical representation for Sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/sync/blob/blob_sync.go`:
- Around line 104-114: The ReSync path in blob synchronization is leaving state
stale because updateState is only called when skipChangeDetection is false,
which causes duplicate publishes on the next normal sync. Update the
sync/publish flow in blob_sync.go so updateState is called unconditionally after
processing a blob, using attrs as nil-safe input when skipChangeDetection is
true; this lets hs.updateState refresh lastBodySHA while preserving
lastETag/lastUpdated. Keep the existing skip-publish branch in sync with
hs.lastBodySHA comparison, and reference sync() plus updateState to ensure the
state is always refreshed after both publish and no-change paths.
- Around line 80-117: The Sync.sync flow reads and updates shared state fields
like lastETag, lastUpdated, and lastBodySHA without any synchronization, which
can race when Poller.Start and ReSync invoke it concurrently. Add mutex or
atomic protection around all accesses to these fields, and update the relevant
methods in Sync such as sync, attributesUnchanged, fetchObject, and updateState
so reads and writes are guarded consistently.
---
Nitpick comments:
In `@core/pkg/sync/blob/blob_sync.go`:
- Around line 183-186: The change-detection hash in generateSha is based on the
raw input bytes, but fetchObject publishes the converted JSON, so equivalent
YAML reformatting can trigger unnecessary updates. Update the hashing flow
around generateSha/fetchObject so the hash is computed from the converted JSON
string (the same payload that gets published), and ensure the comparison uses
that canonical representation for Sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b7cd5131-4649-4f53-a5c0-9a076de6f98e
📒 Files selected for processing (6)
core/pkg/sync/blob/blob_sync.gocore/pkg/sync/blob/blob_sync_test.gocore/pkg/sync/blob/mock_blob.goschemasspectest-harness
|
Initially, I misread the code and thought that we weren't properly updating the bodySha and using it on the next request; we are (I misread) but I think we are missing a test that specifically asserts this. The existing |
| func (hs *Sync) generateSha(body []byte) string { | ||
| hasher := sha3.New256() | ||
| hasher.Write(body) | ||
| return base64.URLEncoding.EncodeToString(hasher.Sum(nil)) |
There was a problem hiding this comment.
Instead of hashing the body directly, hash a canonical representation of the payload; i.e. JSON-parse the data first, then hash the re-serialized output. Go's encoding/json sorts map keys alphabetically and has strong stability guarantees on output. Hashing that means upstream key-ordering differences (not stable per the JSON spec) and YAML comments/whitespace won't cause spurious "changes". Ideally we'd apply this across all sync sources (file, blob, http), not just blob. It's a bit of scope creep, but IMO a good thing to add here.
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/pkg/sync/blob/blob_sync_test.go (1)
237-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to avoid confusion with the
ReSyncmethod.The test name implies that it exercises the
ReSyncmethod, but it actually verifies the complete standard polling update flow (initial publish, changed publish, and an identical tick). Renaming it to align with the PR objective's description will improve clarity for future maintainers.♻️ Proposed refactor
-func TestBlobSync_SyncAndResync(t *testing.T) { +func TestBlobSync_CompleteUpdateFlow(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/sync/blob/blob_sync_test.go` at line 237, Rename the TestBlobSync_SyncAndResync test to reflect that it covers the standard polling update flow rather than the ReSync method, while leaving its test behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/pkg/sync/blob/blob_sync_test.go`:
- Line 237: Rename the TestBlobSync_SyncAndResync test to reflect that it covers
the standard polling update flow rather than the ReSync method, while leaving
its test behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c932618c-fda6-428e-9c32-74f66fe07c6c
📒 Files selected for processing (2)
core/pkg/sync/blob/blob_sync.gocore/pkg/sync/blob/blob_sync_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/pkg/sync/blob/blob_sync.go
leakonvalinka
changed the title
| // based on its attributes alone, allowing us to skip fetching the full object. | ||
| // prefers the ETag and falls back to the mod time for stores that don't expose one. | ||
| func (hs *Sync) attributesUnchanged(attrs *blob.Attributes) bool { | ||
| hs.mu.Lock() |
There was a problem hiding this comment.
since we are adding a locking mechanism, would it make sense to add some kind of small/lightweight concurrency/race-safety test?
e.g. concurrent IsReady(), ReSync(), and regular sync calls on the same Sync, mainly intended to run under go test -race (which is already set up for our tests. see makefile). Just to make sure that there are no race conditions or deadlocks are happening around that data thats protected via the mutex
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/pkg/utils/hash_test.go (2)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix tautological comparison to avoid linter warnings.
Static analysis tools flag
GenerateSha(in) != GenerateSha(in)as an identical sub-expression on both sides. To properly test determinism and appease linters, hash two distinct byte slices containing the same data.♻️ Proposed fix
t.Run("is deterministic for identical input", func(t *testing.T) { - in := []byte(`{"flags":{"a":{"state":"ENABLED"}}}`) - if GenerateSha(in) != GenerateSha(in) { + in1 := []byte(`{"flags":{"a":{"state":"ENABLED"}}}`) + in2 := []byte(`{"flags":{"a":{"state":"ENABLED"}}}`) + if GenerateSha(in1) != GenerateSha(in2) { t.Error("expected identical input to produce identical hashes") } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/utils/hash_test.go` around lines 5 - 11, Update the “is deterministic for identical input” subtest in TestGenerateSha to create two distinct byte slices with identical contents, then compare GenerateSha results for those separate inputs instead of passing the same slice expression twice.Source: Linters/SAST tools
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix tautological comparison to avoid linter warnings.
Similar to the previous test, comparing
GenerateSha([]byte("not json")) != GenerateSha([]byte("not json"))triggers static analysis warnings.♻️ Proposed fix
t.Run("falls back to raw bytes for non-json", func(t *testing.T) { if GenerateSha([]byte("not json")) == GenerateSha([]byte("also not json")) { t.Error("expected distinct non-json payloads to hash differently") } // stable for identical non-json input - if GenerateSha([]byte("not json")) != GenerateSha([]byte("not json")) { + in1 := []byte("not json") + in2 := []byte("not json") + if GenerateSha(in1) != GenerateSha(in2) { t.Error("expected identical non-json input to hash equally") } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/utils/hash_test.go` around lines 37 - 45, Update the “falls back to raw bytes for non-json” test to avoid calling GenerateSha with the identical literal expression on both sides of the comparison. Compute the hash for the repeated non-JSON payload once, then compare that stored result against a separately computed hash to preserve the stability assertion without triggering tautological-comparison warnings.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/utils/hash.go`:
- Around line 3-26: Update canonicalize to parse JSON with json.Decoder
configured with UseNumber(), preserving numeric token text during the subsequent
json.Marshal; retain the existing body fallback when decoding or
canonicalization fails, and keep GenerateSha unchanged.
---
Nitpick comments:
In `@core/pkg/utils/hash_test.go`:
- Around line 5-11: Update the “is deterministic for identical input” subtest in
TestGenerateSha to create two distinct byte slices with identical contents, then
compare GenerateSha results for those separate inputs instead of passing the
same slice expression twice.
- Around line 37-45: Update the “falls back to raw bytes for non-json” test to
avoid calling GenerateSha with the identical literal expression on both sides of
the comparison. Compute the hash for the repeated non-JSON payload once, then
compare that stored result against a separately computed hash to preserve the
stability assertion without triggering tautological-comparison warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: faac19bd-e999-4786-8525-73ff0919758e
📒 Files selected for processing (4)
core/pkg/sync/blob/blob_sync.gocore/pkg/sync/http/http_sync.gocore/pkg/utils/hash.gocore/pkg/utils/hash_test.go
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
leakonvalinka
force-pushed
the
feat/dedupe-unchanged-payloads
branch
from
124c27e to
676cd4b
Compare
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
|
3 participants
This PR
Related Issues
#1988