Conversation
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
2 times, most recently
from
fb8aae0 to
dffee43
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new microbenchmark suite for CrudComponent operations, using purpose-built in-memory stubs to keep execution deterministic and network-free for CI performance regression tracking.
Changes:
- Introduces
crud_bench_test.gowith minimalRetryManager,CollectionResolver,VbucketRouter,KvEndpointClientProvider, andKvClientstubs for benchmarking. - Adds benchmarks for orchestration overhead and a range of CRUD/subdoc/meta/stat operations, including compression and projection/fallback paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
dffee43 to
b851ca9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
51af0ed to
d9d236a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
d9d236a to
92dd19e
Compare
Add comprehensive microbenchmarks for crud.go covering all CRUD operations with deterministic, network-free execution suitable for CI regression detection. Uses minimal purpose-built stubs instead of moq mocks for stability.
Uses NewKvClient() with a custom DialMemdxClientFunc that returns benchMemdClient, exercising actual memdx packet encoding/decoding code paths.
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
92dd19e to
65d4883
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
7fb794b to
93f7c14
Compare
Westwooo
changed the title
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
68ebb1a to
631b26c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cbqueryx/query_int_bench_test.go
Outdated
| if idx := len(hostname) - 1; idx > 0 { | ||
| for i := idx; i >= 0; i-- { | ||
| if hostname[i] == ':' { | ||
| hostname = hostname[:i] | ||
| break | ||
| } |
There was a problem hiding this comment.
The condition if idx := len(hostname) - 1; idx > 0 is unnecessary and adds complexity without benefit. The inner for loop for i := idx; i >= 0; i-- already handles all cases correctly, including when the hostname is empty or has no colon. This differs from the simpler approach in cbsearchx/search_int_bench_test.go (lines 39-44) which directly uses for i := len(hostname) - 1; i >= 0; i--. Consider removing the outer if statement for consistency and simplicity.
| if idx := len(hostname) - 1; idx > 0 { | |
| for i := idx; i >= 0; i-- { | |
| if hostname[i] == ':' { | |
| hostname = hostname[:i] | |
| break | |
| } | |
| for i := len(hostname) - 1; i >= 0; i-- { | |
| if hostname[i] == ':' { | |
| hostname = hostname[:i] | |
| break |
| strings.Contains(err.Error(), "no planPIndexes for index") { | ||
| return false | ||
| } |
There was a problem hiding this comment.
The error handling in the Eventually check may have incorrect logic. When an error occurs that is NOT one of the expected transient errors (ErrNoIndexPartitionsPlanned, ErrNoIndexPartitionsFound, ErrIndexNotFound, or "no planPIndexes for index"), the function returns true, indicating success. This means that unexpected errors will cause the polling to stop and the test to proceed, which could lead to false positives. Consider returning false for unexpected errors or explicitly handling them to ensure the test fails appropriately.
| strings.Contains(err.Error(), "no planPIndexes for index") { | |
| return false | |
| } | |
| strings.Contains(err.Error(), "no planPIndexes for index") { | |
| // Known transient errors: index is not ready yet, keep retrying. | |
| return false | |
| } | |
| // Unexpected error: log and keep retrying until timeout causes failure. | |
| b.Logf("unexpected error while waiting for index %q to become queryable: %v", indexName, err) | |
| return false |
crud_int_bench_test.go
Outdated
| intBenchCreateDocument(ctx, b, agent, key, []byte(benchSmallDoc)) | ||
|
|
||
| // Wait for replication | ||
| time.Sleep(500 * time.Millisecond) |
There was a problem hiding this comment.
The fixed 500ms sleep for waiting for replication is problematic. In a benchmark context, this fixed delay will be included in the setup time before b.ResetTimer() is called, which is correct. However, 500ms may not be sufficient for replication to complete in all environments, leading to flaky benchmarks. Consider either increasing the timeout, implementing a polling mechanism to verify replication completion, or documenting that this benchmark requires a replica to be configured and may be flaky in certain environments.
crud_int_bench_test.go
Outdated
| require.NoError(b, err, "failed to create test document") | ||
| } | ||
|
|
||
| // intBenchCreateCounter creates a counter counter benchmarks |
There was a problem hiding this comment.
The comment has a typo: "counter counter benchmarks" should be "counter for benchmarks".
| // intBenchCreateCounter creates a counter counter benchmarks | |
| // intBenchCreateCounter creates a counter for benchmarks |
Westwooo
force-pushed
the
ING-1422-CI-microbenchmarks
branch
from
631b26c to
7dd4b82
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
| for i := 0; i < b.N; i++ { | ||
| _, err := agent.Delete(ctx, &gocbcorex.DeleteOptions{ | ||
| Key: []byte(keys[i]), | ||
| ScopeName: "", | ||
| CollectionName: "", | ||
| }) |
There was a problem hiding this comment.
This benchmark converts keys[i] from string to []byte inside the timed loop ([]byte(keys[i])), which allocates every iteration and skews ReportAllocs/throughput. Precompute [][]byte keys (or reuse a single options struct and update its Key) before ResetTimer().
| b.ResetTimer() | |
| b.ReportAllocs() | |
| for i := 0; i < b.N; i++ { | |
| _, err := agent.Delete(ctx, &gocbcorex.DeleteOptions{ | |
| Key: []byte(keys[i]), | |
| ScopeName: "", | |
| CollectionName: "", | |
| }) | |
| // Precompute []byte keys to avoid per-iteration allocations in the benchmark loop | |
| keyBytes := make([][]byte, b.N) | |
| for i := 0; i < b.N; i++ { | |
| keyBytes[i] = []byte(keys[i]) | |
| } | |
| // Reuse a single DeleteOptions instance across iterations | |
| opts := &gocbcorex.DeleteOptions{ | |
| ScopeName: "", | |
| CollectionName: "", | |
| } | |
| b.ResetTimer() | |
| b.ReportAllocs() | |
| for i := 0; i < b.N; i++ { | |
| opts.Key = keyBytes[i] | |
| _, err := agent.Delete(ctx, opts) |
| _, err := agent.Unlock(ctx, &gocbcorex.UnlockOptions{ | ||
| Key: []byte(keys[i]), | ||
| ScopeName: "", | ||
| CollectionName: "", | ||
| Cas: casValues[i], | ||
| }) |
There was a problem hiding this comment.
This benchmark converts keys[i] from string to []byte inside the timed loop ([]byte(keys[i])), which allocates every iteration and skews ReportAllocs. Precompute [][]byte keys (and/or options structs) before ResetTimer().
| _, err := agent.GetAndLock(ctx, &gocbcorex.GetAndLockOptions{ | ||
| Key: []byte(keys[i]), | ||
| ScopeName: "", | ||
| CollectionName: "", | ||
| LockTime: 30, | ||
| }) |
There was a problem hiding this comment.
This benchmark converts keys[i] from string to []byte inside the timed loop ([]byte(keys[i])), which allocates every iteration and skews ReportAllocs. Precompute [][]byte keys (and/or options structs) before ResetTimer().
| _, err := agent.Add(ctx, &gocbcorex.AddOptions{ | ||
| Key: []byte(keys[i]), | ||
| ScopeName: "", | ||
| CollectionName: "", | ||
| Value: []byte(benchDoc1KB), | ||
| }) |
There was a problem hiding this comment.
This benchmark converts keys[i] from string to []byte inside the timed loop ([]byte(keys[i])), which allocates every iteration and skews ReportAllocs. Precompute [][]byte keys (and/or options structs) before ResetTimer().
| _, err := agent.AddWithMeta(ctx, &gocbcorex.AddWithMetaOptions{ | ||
| Key: []byte(keys[i]), | ||
| ScopeName: "", | ||
| CollectionName: "", | ||
| Value: []byte(benchSmallDoc), | ||
| StoreCas: 1, | ||
| }) |
There was a problem hiding this comment.
This benchmark converts keys[i] from string to []byte inside the timed loop ([]byte(keys[i])), which allocates every iteration and skews ReportAllocs. Precompute [][]byte keys (and/or options structs) before ResetTimer().
| _, err := agent.DeleteWithMeta(ctx, &gocbcorex.DeleteWithMetaOptions{ | ||
| Key: []byte(keys[i]), | ||
| ScopeName: "", | ||
| CollectionName: "", | ||
| StoreCas: 1, | ||
| Options: memdx.MetaOpFlagSkipConflictResolution, | ||
| }) |
There was a problem hiding this comment.
This benchmark converts keys[i] from string to []byte inside the timed loop ([]byte(keys[i])), which allocates every iteration and skews ReportAllocs. Precompute [][]byte keys (and/or options structs) before ResetTimer().
Add comprehensive microbenchmarks for crud.go covering all CRUD operations with deterministic, network-free execution suitable for CI regression detection. Uses minimal purpose-built stubs instead of moq mocks for stability.