Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands Elastickv's capabilities by integrating a fully functional S3-compatible API server. This new feature allows users to interact with Elastickv as a distributed object store, supporting standard S3 operations and ensuring data integrity and security through AWS SigV4 authentication. The changes involve substantial additions to the adapter layer, new key management utilities, and enhancements to the core transaction and sharding mechanisms to seamlessly support S3 workloads. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: an S3-compatible API layer. The implementation is comprehensive, covering the server, request handling, SigV4 authentication, data storage using the existing MVCC store, and sharding logic. The code is well-structured with a good separation of concerns. The changes also include necessary updates to the coordinator and sharding layers to support S3 data types and transactional semantics. The addition of tests, command-line flags, and Grafana dashboards rounds out a very complete feature implementation. My review has identified a couple of areas for improvement, including a potential performance issue in object listing and a minor clarity issue in a utility function.
adapter/s3.go
Outdated
| if remaining := maxKeys - result.KeyCount; remaining > pageLimit { | ||
| pageLimit = remaining | ||
| } |
There was a problem hiding this comment.
The logic for calculating pageLimit appears to be inverted. It currently uses > which results in pageLimit being set to max(s3ListPageSize, remaining). This could lead to large, inefficient scans against the underlying store, bypassing the intended paging behavior.
To fix this, the comparison should be < to correctly cap the page size at the number of remaining keys if it's smaller than the default page size.
| if remaining := maxKeys - result.KeyCount; remaining > pageLimit { | |
| pageLimit = remaining | |
| } | |
| if remaining := maxKeys - result.KeyCount; remaining < pageLimit { | |
| pageLimit = remaining | |
| } |
adapter/s3.go
Outdated
| if millis > math.MaxInt64 { | ||
| millis = math.MaxInt64 | ||
| } | ||
| return time.UnixMilli(int64(millis)).UTC() //nolint:gosec // millis is clamped to MaxInt64 above. |
There was a problem hiding this comment.
The //nolint:gosec comment here is a bit ambiguous. While the following comment correctly explains why the uint64 to int64 conversion is safe, the nolint directive itself doesn't specify which gosec rule is being suppressed. This can be confusing for future maintenance. For better clarity, consider specifying the exact rule being suppressed (e.g., //nolint:gosec // Gxxx: explanation) or removing the directive if it's no longer flagged by your linter.
There was a problem hiding this comment.
Pull request overview
Adds an S3-compatible API surface to elastickv (including routing/sharding support via new S3 keyspace utilities), extends transaction coordination to optionally accept a caller-provided commit timestamp, and updates Grafana dashboards to better scope Raft/DynamoDB views (including leader-scoped Raft context).
Changes:
- Introduce S3 adapter server with SigV4 auth, leader proxying, bucket/object operations, and supporting tests.
- Add
CommitTStoOperationGroupand plumb it through coordinators (single + sharded) with new test coverage. - Add
internal/s3keyskey format + routing helpers; update shard routing for S3 manifest scans; add new/updated Grafana dashboards.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| monitoring/grafana/dashboards/elastickv-raft-status.json | Adjusts Raft dashboard queries to be group-filterable and leader-scoped; adds table transformations. |
| monitoring/grafana/dashboards/elastickv-dynamodb-summary.json | New DynamoDB activity dashboard including leader-scoped Raft membership context. |
| monitoring/grafana/dashboards/elastickv-cluster-summary.json | New compact dashboard combining DynamoDB activity and core Raft state. |
| monitoring/grafana/dashboards/elastickv-cluster-overview.json | Updates existing overview dashboard datasource config and leader-scoped Raft tables. |
| kv/transcoder.go | Adds optional CommitTS field to OperationGroup. |
| kv/sharded_coordinator.go | Plumbs optional CommitTS through sharded txn dispatch/log building. |
| kv/sharded_coordinator_txn_test.go | Adds test ensuring sharded coordinator honors provided CommitTS. |
| kv/shard_store_txn_lock_test.go | Adds cross-shard txn lock/scan resolution tests. |
| kv/shard_store_test.go | Adds sharded scan tests for S3 manifest key prefixes and routing behavior. |
| kv/shard_store.go | Routes S3 manifest scans based on logical S3 route bounds (not raw key prefixes). |
| kv/shard_key.go | Normalizes S3 keys (and txn-wrapped S3 keys) to stable route keys for sharding. |
| kv/shard_key_test.go | Tests route-key normalization for S3 keys (manifest/blob/txn-wrapped). |
| kv/coordinator.go | Adds optional CommitTS handling for txn dispatch and redirects. |
| kv/coordinator_txn_test.go | Adds test ensuring coordinator honors provided CommitTS. |
| internal/s3keys/keys.go | Implements S3 key formats, segment escaping, route-key extraction, and scan route bounds. |
| internal/s3keys/keys_test.go | Adds round-trip and routing bound tests for S3 key encoding/parsing. |
| cmd/server/demo.go | Wires up an S3 server in demo/server with flags, leader map plumbing, and static credential loading. |
| cmd/server/demo_test.go | Adds tests for S3 credential loading and path-style-only enforcement. |
| adapter/s3_auth.go | Adds SigV4 auth enforcement when static credentials are configured. |
| adapter/s3.go | Implements S3 server handlers, proxy-to-leader behavior, persistence layout, and retry behavior. |
| adapter/s3_test.go | Adds end-to-end tests for S3 API behavior, proxying, SigV4 auth, and sharded routing. |
| adapter/dynamodb_migration_test.go | Updates local test coordinator to support pinned CommitTS and factor commit/apply logic. |
adapter/s3.go
Outdated
|
|
||
| for result.KeyCount < maxKeys { | ||
| pageLimit := s3ListPageSize | ||
| if remaining := maxKeys - result.KeyCount; remaining > pageLimit { |
There was a problem hiding this comment.
In ListObjectsV2 pagination, the pageLimit calculation is inverted: when remaining > pageLimit it sets pageLimit = remaining, which can grow the scan batch beyond s3ListPageSize and defeat bounded paging. This should clamp the page size to the smaller of s3ListPageSize and remaining.
| if remaining := maxKeys - result.KeyCount; remaining > pageLimit { | |
| if remaining := maxKeys - result.KeyCount; remaining < pageLimit { |
adapter/s3.go
Outdated
| func (s *S3Server) cleanupManifestBlobsAsync(bucket string, generation uint64, objectKey string, manifest *s3ObjectManifest) { | ||
| if manifest == nil { | ||
| return | ||
| } | ||
| go s.cleanupManifestBlobs(context.Background(), bucket, generation, objectKey, manifest) |
There was a problem hiding this comment.
cleanupManifestBlobsAsync spawns an unbounded goroutine with context.Background(). Under high churn (overwrites/deletes), this can create many long-lived goroutines and background work that cannot be cancelled on shutdown. Consider using a bounded worker/errgroup, or passing a context with timeout / server-level shutdown context so cleanup can be throttled and cancelled.
| func (s *S3Server) cleanupManifestBlobsAsync(bucket string, generation uint64, objectKey string, manifest *s3ObjectManifest) { | |
| if manifest == nil { | |
| return | |
| } | |
| go s.cleanupManifestBlobs(context.Background(), bucket, generation, objectKey, manifest) | |
| const s3ManifestCleanupTimeout = 2 * time.Minute | |
| func (s *S3Server) cleanupManifestBlobsAsync(bucket string, generation uint64, objectKey string, manifest *s3ObjectManifest) { | |
| if manifest == nil { | |
| return | |
| } | |
| go func() { | |
| ctx, cancel := context.WithTimeout(context.Background(), s3ManifestCleanupTimeout) | |
| defer cancel() | |
| s.cleanupManifestBlobs(ctx, bucket, generation, objectKey, manifest) | |
| }() |
| { | ||
| "datasource": { | ||
| "type": "prometheus", | ||
| "uid": null | ||
| }, |
There was a problem hiding this comment.
This dashboard mixes panel datasource representations: early panels use { "type": "prometheus", "uid": null } while later panels use datasource: null. Mixing formats can cause unexpected import/override behavior in Grafana; consider normalizing to a single approach (matching the other dashboards in this repo).
| Message: "credential scope date does not match x-amz-date", | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
SigV4 authorization currently parses X-Amz-Date but does not enforce any clock-skew/expiration window. That makes captured signed requests replayable indefinitely (until credentials rotate). Consider rejecting requests where X-Amz-Date is too far from time.Now().UTC() (e.g., >15m) to align with AWS SigV4 replay protections.
| // Enforce a clock-skew window for the SigV4 signing time to limit replay. | |
| const maxClockSkew = 15 * time.Minute | |
| now := time.Now().UTC() | |
| skew := now.Sub(signingTime) | |
| if skew < 0 { | |
| skew = -skew | |
| } | |
| if skew > maxClockSkew { | |
| return &s3AuthError{ | |
| Status: http.StatusForbidden, | |
| Code: "RequestTimeTooSkewed", | |
| Message: "The difference between the request time and the server's time is too large", | |
| } | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
kv/coordinator.go:119
- When a non-zero CommitTS is provided, the coordinator uses it without updating/observing the local HLC. This can allow subsequent calls to issue timestamps < CommitTS (since HLC.Next() is based on its internal last value), which can break MVCC ordering and make later writes effectively invisible. Consider calling c.clock.Observe(commitTS) (and possibly validating commitTS > c.clock.Current()) when CommitTS is supplied.
if commitTS == 0 {
commitTS = c.clock.Next()
if commitTS <= startTS {
c.clock.Observe(startTS)
commitTS = c.clock.Next()
}
}
if commitTS <= startTS {
return nil, errors.WithStack(ErrTxnCommitTSRequired)
}
| if commitTS == 0 { | ||
| commitTS = c.nextTxnTSAfter(startTS) | ||
| } | ||
| if commitTS == 0 || commitTS <= startTS { | ||
| return nil, errors.WithStack(ErrTxnCommitTSRequired) | ||
| } |
There was a problem hiding this comment.
Similar to the single-group coordinator: if commitTS is provided, it is used without observing it on c.clock. That can allow the HLC to later issue timestamps smaller than a previously used CommitTS, violating monotonic MVCC ordering. Consider observing the provided commitTS (and/or validating it against c.clock.Current()) when commitTS != 0.
| commitTS := reqs.CommitTS | ||
| if commitTS == 0 { | ||
| commitTS = c.nextTxnTSAfter(reqs.StartTS) | ||
| } | ||
| if commitTS == 0 || commitTS <= reqs.StartTS { | ||
| return nil, errors.WithStack(ErrTxnCommitTSRequired) | ||
| } |
There was a problem hiding this comment.
txnLogs accepts a non-zero reqs.CommitTS without observing it on the HLC. If callers use this compatibility path with pinned commit timestamps, the coordinator can later issue smaller timestamps than a previous CommitTS, breaking MVCC ordering. Consider observing/validating reqs.CommitTS when it is non-zero.
| expectedAuth, err := buildS3AuthorizationHeader(r, parsed.AccessKeyID, secretAccessKey, s.effectiveRegion(), signingTime, payloadHash) | ||
| if err != nil { | ||
| return &s3AuthError{ | ||
| Status: http.StatusForbidden, | ||
| Code: "AccessDenied", | ||
| Message: "failed to verify request signature", | ||
| } | ||
| } | ||
| if subtle.ConstantTimeCompare([]byte(authHeader), []byte(expectedAuth)) != 1 { | ||
| return &s3AuthError{ | ||
| Status: http.StatusForbidden, | ||
| Code: "SignatureDoesNotMatch", | ||
| Message: "request signature does not match", | ||
| } | ||
| } |
There was a problem hiding this comment.
SigV4 verification currently compares the entire Authorization header string against a freshly-signed header. This is brittle: equivalent SigV4 requests that differ only in parameter ordering/whitespace in the Authorization header (but have the same signature) may be rejected. Consider parsing out and comparing the Signature (and ensuring Credential scope matches), rather than requiring full string equality.
| var requests []*pb.Request | ||
| if reqs.IsTxn { | ||
| primary := primaryKeyForElems(reqs.Elems) | ||
| if len(primary) == 0 { | ||
| return nil, errors.WithStack(ErrTxnPrimaryKeyRequired) | ||
| } | ||
| requests = []*pb.Request{ | ||
| onePhaseTxnRequest(reqs.StartTS, 0, primary, reqs.Elems), | ||
| onePhaseTxnRequest(reqs.StartTS, reqs.CommitTS, primary, reqs.Elems), | ||
| } |
There was a problem hiding this comment.
In the redirect path, a txn with CommitTS set but StartTS still 0 is forwarded as-is. The leader-side Internal.Forward code will stamp a StartTS independently but will not override an already-nonzero CommitTS in the txn meta mutation, so it can end up with CommitTS <= StartTS (invalid txn) or inconsistent timestamps. Consider rejecting CommitTS-without-StartTS at the API boundary, or ensure StartTS is set consistently before forwarding, or let the leader fill CommitTS by forwarding CommitTS=0 when StartTS is unset.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…dinator redirect Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/26a0122b-27bb-4ba7-8596-ae3af70b98dd
Fix HLC monotonicity, SigV4 sig comparison, and coordinator redirect timestamp safety
| if commitTS == 0 { | ||
| commitTS = c.clock.Next() | ||
| if commitTS <= startTS { | ||
| c.clock.Observe(startTS) | ||
| commitTS = c.clock.Next() | ||
| } | ||
| } | ||
| if commitTS <= startTS { | ||
| return nil, errors.WithStack(ErrTxnCommitTSRequired) |
There was a problem hiding this comment.
When a non-zero commitTS is provided, the coordinator clock is not advanced/observed. If commitTS is greater than the local HLC state, subsequent c.clock.Next() calls can return timestamps < commitTS, breaking the monotonic timestamp guarantee across transactions. Consider calling c.clock.Observe(commitTS) (or at least when commitTS > c.clock.Current()) before returning, mirroring the sharded coordinator behavior.
| func setupS3( | ||
| ctx context.Context, | ||
| lc net.ListenConfig, | ||
| st store.MVCCStore, | ||
| coordinator *kv.Coordinate, | ||
| addr string, | ||
| s3Addr string, | ||
| raftS3MapStr string, | ||
| region string, | ||
| credentialsFile string, | ||
| pathStyleOnly bool, | ||
| readTracker *kv.ActiveTimestampTracker, | ||
| ) (*adapter.S3Server, error) { | ||
| if !pathStyleOnly { | ||
| return nil, errors.New("virtual-hosted style S3 requests are not implemented") | ||
| } |
There was a problem hiding this comment.
setupS3 accepts coordinator as *kv.Coordinate, but if a nil coordinator is ever passed while pathStyleOnly is true, the typed-nil pointer will be stored in interfaces (LeaderRoutedStore / S3Server) and can cause panics when methods are invoked. Consider explicitly validating coordinator != nil here and returning an error (or changing the parameter type to kv.Coordinator and passing a real nil interface when absent).
| if raftS3MapStr != "" { | ||
| parts := strings.SplitSeq(raftS3MapStr, ",") | ||
| for part := range parts { | ||
| kv := strings.Split(part, "=") | ||
| if len(kv) == kvParts { | ||
| leaderS3[raft.ServerAddress(kv[0])] = kv[1] | ||
| } | ||
| } |
There was a problem hiding this comment.
The raftS3MapStr parsing uses strings.Split(part, "=") and ignores malformed entries (len != kvParts) silently. This will also mis-handle values containing '=' and makes debugging configuration issues harder. Consider using strings.SplitN(part, "=", kvParts) + strings.TrimSpace on both sides, and returning an error for invalid entries (similar to parseRaftRedisMap in shard_config.go).
| return s | ||
| } | ||
|
|
||
| func (s *S3Server) Run() error { |
There was a problem hiding this comment.
S3Server.Run calls s.httpServer.Serve(s.listen) without validating that s.listen is non-nil. If NewS3Server is constructed without a listener (as some tests do), calling Run would panic. Consider returning a descriptive error when s == nil, s.httpServer == nil, or s.listen == nil.
| func (s *S3Server) Run() error { | |
| func (s *S3Server) Run() error { | |
| if s == nil { | |
| return errors.New("s3 server is nil") | |
| } | |
| if s.httpServer == nil { | |
| return errors.New("s3 server httpServer is nil") | |
| } | |
| if s.listen == nil { | |
| return errors.New("s3 server listener is nil") | |
| } |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…rsing hardening Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/82513c62-6b3f-4606-9c2f-4f1fbaa470ad
Fix HLC monotonicity, nil guards, and config parsing in S3 adapter
| func writeS3InternalError(w http.ResponseWriter, err error) { | ||
| message := "internal error" | ||
| if err != nil { | ||
| message = err.Error() | ||
| } | ||
| writeS3Error(w, http.StatusInternalServerError, "InternalError", message, "", "") | ||
| } |
There was a problem hiding this comment.
writeS3InternalError returns the raw underlying error message to the client. This can leak internal details (storage keys, topology, file paths, etc.) and differs from typical S3 behavior. Prefer logging the detailed error server-side and returning a generic "InternalError" message to clients.
| } | ||
| if req.IsTxn && commitTS <= req.StartTS { | ||
| return 0, kv.ErrTxnCommitTSRequired | ||
| } |
There was a problem hiding this comment.
localAdapterCoordinator.commitTSForRequest accepts a caller-provided CommitTS but does not Observe() it on the HLC. If a test provides a CommitTS ahead of the coordinator clock, subsequent Next() calls can go backwards relative to that CommitTS, which can make timestamp-dependent behavior flaky or diverge from production coordinator behavior. Consider observing non-zero CommitTS (and/or StartTS) to keep the test clock monotonic.
| } | |
| } | |
| if commitTS != 0 { | |
| c.Clock().Observe(commitTS) | |
| } |
| n, readErr := r.Body.Read(buf) | ||
| if n > 0 { | ||
| chunk := append([]byte(nil), buf[:n]...) | ||
| if _, err := hasher.Write(chunk); err != nil { | ||
| writeS3InternalError(w, err) | ||
| return | ||
| } | ||
| if validatePayloadSHA { | ||
| if _, err := sha256Hasher.Write(chunk); err != nil { | ||
| writeS3InternalError(w, err) |
There was a problem hiding this comment.
S3 PUT currently reads the entire request body without any explicit size limit. Unlike the DynamoDB adapter (which wraps the body with MaxBytesReader), this leaves the server vulnerable to very large uploads causing excessive memory/disk usage and long-lived connections. Consider enforcing a maximum object size (or at least a configurable cap) and returning an appropriate S3 error when exceeded.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…es=false Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/f96f5f26-a7f9-4563-b9a8-db083ce32609
…ReverseScanResults Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/845127a7-c957-4c22-8699-b8584a84033d
kv: fix ReverseScanAt cross-shard ordering when clampToRoutes=false
adapter/s3.go
Outdated
|
|
||
| func (s *S3Server) pinReadTS(ts uint64) *kv.ActiveTimestampToken { | ||
| if s == nil || s.readTracker == nil { | ||
| return nil |
There was a problem hiding this comment.
pinReadTS can return nil when readTracker isn't configured, but callers unconditionally do defer readPin.Release() (e.g. listBuckets, createBucket, headBucket, etc.). This will panic at runtime in configurations/tests that don't set WithS3ActiveTimestampTracker. Consider returning a no-op token (e.g. &kv.ActiveTimestampToken{}) instead of nil, or guarding the Release() calls.
| return nil | |
| // When read tracking is not configured, return a no-op token so callers | |
| // can safely defer readPin.Release() without panicking. | |
| return &kv.ActiveTimestampToken{} |
adapter/s3.go
Outdated
| if s.leaderS3 == nil { | ||
| s.leaderS3 = map[raft.ServerAddress]string{} | ||
| } | ||
| if leader := s.coordinatorLeaderAddress(); leader != "" { | ||
| s.leaderS3[leader] = s.s3Addr |
There was a problem hiding this comment.
NewS3Server unconditionally sets leaderS3[coordinatorLeaderAddress()] = s3Addr when s3Addr is non-empty. On follower nodes (where coordinatorLeaderAddress() points at the current leader), this overwrites the leader’s correct S3 endpoint with the follower’s local s3Addr, causing maybeProxyToLeader to proxy requests back to the wrong host. Consider only setting this mapping when no leaderS3 map was provided (or when the leader entry is absent), or require the caller to pass the full raft->S3 mapping explicitly.
| if s.leaderS3 == nil { | |
| s.leaderS3 = map[raft.ServerAddress]string{} | |
| } | |
| if leader := s.coordinatorLeaderAddress(); leader != "" { | |
| s.leaderS3[leader] = s.s3Addr | |
| if leader := s.coordinatorLeaderAddress(); leader != "" { | |
| if s.leaderS3 == nil { | |
| s.leaderS3 = map[raft.ServerAddress]string{} | |
| } | |
| if _, exists := s.leaderS3[leader]; !exists { | |
| s.leaderS3[leader] = s.s3Addr | |
| } |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…lts, dedup by GroupID, pinReadTS no-op token, guard leader S3 map entry Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/2f32e04f-12e2-4392-9ee8-2f1291e6b67f
Fix shard reverse scan ordering, GroupID dedup, pinReadTS nil panic, and leader S3 map overwrite
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| route := routes[i] | ||
| scanStart := start | ||
| scanEnd := end | ||
| if clampToRoutes { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶if clampToRoutes has complex nested blocks (complexity: 5) (nestif)
| if reqs.IsTxn && reqs.StartTS == 0 { | ||
| // Leader-only timestamp issuance to avoid divergence across shards. | ||
| reqs.StartTS = c.nextStartTS() | ||
| } | ||
|
|
||
| if reqs.IsTxn { | ||
| return c.dispatchTxn(reqs.Elems, reqs.StartTS) | ||
| return c.dispatchTxn(reqs.Elems, reqs.StartTS, reqs.CommitTS) | ||
| } |
There was a problem hiding this comment.
When StartTS is auto-assigned (StartTS==0), a caller-provided CommitTS is still forwarded into dispatchTxn. This can create invalid transactions (CommitTS <= newly-assigned StartTS) and fails the monotonicity intent. Consider clearing reqs.CommitTS when you assign reqs.StartTS (same rationale as the redirect() handling), so the leader chooses both timestamps consistently.
| if reqs.IsTxn && reqs.StartTS == 0 { | ||
| startTS, err := c.nextStartTS(ctx, reqs.Elems) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| reqs.StartTS = startTS | ||
| } | ||
|
|
||
| if reqs.IsTxn { | ||
| return c.dispatchTxn(reqs.StartTS, reqs.Elems) | ||
| return c.dispatchTxn(reqs.StartTS, reqs.CommitTS, reqs.Elems) |
There was a problem hiding this comment.
If StartTS is auto-assigned (StartTS==0) but CommitTS is caller-provided, dispatchTxn will validate CommitTS against the new StartTS and may reject the txn (or observe an arbitrary external timestamp). Consider zeroing CommitTS whenever the coordinator assigns StartTS so both timestamps are generated consistently (mirrors the redirect() logic in kv/Coordinate).
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootjp/elastickv/sessions/4953ec80-5e46-4a9f-8aad-5babc83f3f41
kv: zero CommitTS when StartTS is auto-assigned in coordinators
No description provided.