Add offset shift vector check #2818
Conversation
📝 WalkthroughWalkthroughThis update refines vector comparison in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as GetObject Method
C->>G: Request object
G->>G: Retrieve response vector (using resp.GetVector())
G->>G: Compare received vector with expected vector
alt Exact match or shifted match (1 or 2 elements)
G->>C: Return object
else No matching ordering
G->>G: Log error "vector mismatch"
G->>C: Return error
end
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
size/S
type/bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/operation/stream.go (1)
1151-1155: LGTM! Consider enhancing the error message.The implementation correctly handles the offset shift vector check by comparing against the original vector and its shifted variations. However, the error message could be more informative.
Consider improving the error message to indicate which variation was expected to match:
- t.Errorf( - "got: %#v, expected: %#v", - resp.GetVector(), - ds.Train[idx], - ) + t.Errorf( + "got: %#v, expected one of:\noriginal: %#v\nshift1: %#v\nshift2: %#v", + resp.GetVector(), + v, + append(v[1:], v[:1]...), + append(v[2:], v[:2]...), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/operation/stream.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (41)
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: detect-ci-container / detect
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
- GitHub Check: E2E test (Stream CRUD) for operator
- GitHub Check: E2E test (Stream CRUD)
- GitHub Check: detect-ci-container / detect
- GitHub Check: dump-contexts-to-log
Profile Report
|
Profile Report
|
vankichi
changed the title
Deploying vald with
|
| Latest commit: |
cc2be82
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8a602b1e.vald.pages.dev |
| Branch Preview URL: | https://bugfix-e2e-add-getobject-off.vald.pages.dev |
kmrmt
requested review from
a team,
vankichi and
datelier
and removed request for
a team
kmrmt
force-pushed
the
bugfix/e2e/add-getobject-offset-check
branch
from
586b933 to
af3119c
Compare
kmrmt
force-pushed
the
bugfix/e2e/add-getobject-offset-check
branch
2 times, most recently
from
47ed14c to
5d0aa64
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/operation/stream.go (1)
1153-1158: Consider enhancing the error message for better debugging.The current error message shows the vectors but doesn't indicate which comparison failed (original or shifted). Consider making it more descriptive.
- t.Errorf( - "got: %#v, expected: %#v", - resp.GetVector(), - ds.Train[idx], - ) + t.Errorf( + "vector mismatch - got: %#v, expected one of:\noriginal: %#v\nshift-1: %#v\nshift-2: %#v", + resp.GetVector(), + v, + append(v[1:], v[:1]...), + append(v[2:], v[:2]...), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/operation/stream.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
tests/e2e/operation/stream.go (1)
1151-1152: LGTM! Enhanced vector comparison with offset shift checks.The implementation correctly handles vector comparison with offset shifts, improving test coverage by verifying vectors in their original form and with shifted elements.
Also applies to: 1155-1155
kmrmt
dismissed stale reviews from datelier, coderabbitai[bot], and vankichi
via
dc4f8ea
kmrmt
force-pushed
the
bugfix/e2e/add-getobject-offset-check
branch
from
5d0aa64 to
dc4f8ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
hack/go.mod.default (1)
149-149: 💡 Verification agent🧩 Analysis chain
Verify stability of master branch dependencies.
Some dependencies are pointing to master/main branches which could lead to instability:
- github.com/google/btree -> master
- github.com/klauspost/compress -> master
Consider pinning these to specific versions for better stability and reproducibility.
Also applies to: 195-195
🏁 Script executed:
#!/bin/bash # Check if these repositories have recent releases that could be used instead of master gh api repos/google/btree/releases/latest --jq .tag_name gh api repos/klauspost/compress/releases/latest --jq .tag_nameLength of output: 287
I'll gather additional information about the available tags for
github.com/google/btreeto check if there are any stable version tags that could be used instead of the master branch.
🏁 Script executed:
#!/bin/bash # List all tags for the google/btree repository gh api repos/google/btree/tags --jq ".[].name"Length of output: 90
Replace master branch dependencies with stable versions
- For github.com/google/btree, while there is no "latest release" returned, the repository has stable tags available (with v1.1.3 being the most recent).
- For github.com/klauspost/compress, the latest stable release is v1.17.11.
It is advisable to update the dependency entries in
hack/go.mod.default(lines 149 and 195) to use these stable versions instead of referencing the master branch to improve reproducibility and stability.
🧹 Nitpick comments (1)
hack/go.mod.default (1)
6-331: Consider automating dependency updates.Most dependencies are marked with "upgrade" placeholder. Consider using tools like Dependabot or Renovate to automate dependency updates and security patches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
charts/vald-benchmark-operator/schemas/job-values.yaml(0 hunks)charts/vald-benchmark-operator/schemas/scenario-values.yaml(0 hunks)charts/vald-benchmark-operator/values.yaml(0 hunks)charts/vald-helm-operator/values.yaml(0 hunks)charts/vald/schemas/mirror-target-values.yaml(0 hunks)charts/vald/values.yaml(0 hunks)example/client/go.mod(3 hunks)example/client/go.mod.default(1 hunks)go.mod(8 hunks)hack/go.mod.default(1 hunks)
💤 Files with no reviewable changes (6)
- charts/vald/schemas/mirror-target-values.yaml
- charts/vald-benchmark-operator/schemas/job-values.yaml
- charts/vald-benchmark-operator/schemas/scenario-values.yaml
- charts/vald-helm-operator/values.yaml
- charts/vald-benchmark-operator/values.yaml
- charts/vald/values.yaml
✅ Files skipped from review due to trivial changes (1)
- example/client/go.mod.default
🚧 Files skipped from review as they are similar to previous changes (2)
- example/client/go.mod
- go.mod
🔇 Additional comments (4)
hack/go.mod.default (4)
16-16: LGTM: Version pinned with explanation.The version for
cloud.google.com/go/storageis correctly pinned to v1.47.0 with a reference to the issue (#11283) explaining why.
134-134: LGTM: Version pinned with explanation.The version for
github.com/goccy/go-jsonis correctly pinned to v0.10.3 with a reference to the issue (#529) explaining why.
332-341: LGTM: Kubernetes dependencies are properly versioned.All Kubernetes dependencies are correctly pinned to v0.32.0, ensuring consistency across the k8s ecosystem.
3-3: Verify Go version compatibility.The Go version has been updated to 1.24.0. Please ensure all dependencies and code are compatible with this version.
✅ Verification successful
🌐 Web query:
Is Go 1.24.0 released and stable? What are the breaking changes?💡 Result:
Go 1.24 was officially released on February 11, 2025[1][4], and is considered stable based on its official release status and inclusion of performance improvements[1][9]. Here are the key details:
Stability & Release Status
- Stable release: Available for download from go.dev/dl[1]
- Maintained compatibility: Maintains Go 1 compatibility promise for existing code[9]
- Support timeline: Supported until two subsequent major releases (likely until Go 1.26)[4]
Breaking Changes
While most changes maintain backward compatibility, these could impact existing code:
Compiler Restrictions
- Methods can no longer be defined on cgo-generated types via aliases[9]
Example error case:type MyCType C.struct_original // Previously allowed via alias func (m *MyCType) NewMethod() {} // Now causes compiler errorLinker Behavior
- Default generation of build IDs/UUIDs on ELF/macOS platforms[9]
This affects reproducible builds unless disabled with-B nonelinker flagFIPS Configuration
- Deprecated
GOFIPS=1in favor ofGODEBUG=fips140=on[5]
Old method will be removed in Go 1.25Runtime Experiments
New map/mutex implementations enabled by default but can be disabled via:GOEXPERIMENT="noswissmap,nospinbitmutex"Notable Non-Breaking Changes
- Performance: ~2-3% CPU overhead reduction from new map implementation and memory optimizations[1][9]
- WebAssembly: WASI reactor support and
go:wasmexportdirective[1]- Testing: New
testing.B.Loopbenchmark pattern replaces manualb.Nloops[1]For most users upgrading from Go 1.23 or earlier versions, no code changes should be required unless using cgo type aliases or relying on linker build ID behavior[1][9]. Always test with
go vetand check your CI/CD pipelines for reproducibility flags if needed[9].Citations:
- 1: https://go.dev/blog/go1.24
- 2: https://go.dev/doc/devel/release
- 3: https://github.com/uber-go/zap/blob/master/CHANGELOG.md
- 4: https://endoflife.date/go
- 5: https://devblogs.microsoft.com/go/go-1-24-fips-update/
- 6: https://tip.golang.org/doc/go1.23
- 7: https://groups.google.com/g/golang-dev/c/K4ThcNn03cQ
- 8: https://devcenter.upsun.com/posts/go-124/
- 9: https://tip.golang.org/doc/go1.24
- 10: doc: write release notes for Go 1.24 golang/go#68545
Go 1.24.0 is confirmed as stable and officially released.
Your module filehack/go.mod.defaultspecifyinggo 1.24.0aligns with the stable release. Notable breaking changes (e.g., restrictions on cgo alias types and certain linker behavior) are documented, so ensure your code and dependencies don’t rely on features affected by these changes.
- Verified that Go 1.24.0 is stable as of its February 11, 2025 release.
- No issues found directly in the module file; however, please test your full codebase for compatibility with any breaking changes.
kmrmt
force-pushed
the
bugfix/e2e/add-getobject-offset-check
branch
from
a8b7a62 to
41b270a
Compare
kmrmt
force-pushed
the
bugfix/e2e/add-getobject-offset-check
branch
from
41b270a to
65d83a8
Compare
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Tests
Documentation
Chores