Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request relocates event sorting logic from application-level (in-memory sorting in etcd.go) to database-level (etcd client-side sorting in pending_event/store.go), removing the sortPendingEvents function and consolidating the sorting operation into a single clientv3 query parameter. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/internal/workflows/backend/etcd/pending_event/store.go`:
- Around line 51-53: GetByInstanceExecution currently relies only on
clientv3.SortByCreateRevision which can produce nondeterministic ordering when
multiple events share the same CreateRevision; update the fetch to guarantee
deterministic ordering by applying a secondary tie-breaker: either request a
stable secondary sort by key (e.g., include SortByKey/SortAscend if the client
supports composing sorts) or, more robustly, perform a client-side stable sort
of the returned KVs by (CreateRevision, Key) before converting to Value. Locate
Store.GetByInstanceExecution (uses InstanceExecutionPrefix and
storage.NewGetPrefixOp) and modify the retrieval flow so the final ordering is
stable by CreateRevision then Key (or implement the stable-sorting logic in the
GetMultipleOp processing path that wraps NewGetPrefixOp).
d333f55 to
146650c
Compare
1fc7985 to
7264b7e
Compare
Changes the post-query pending event sort to use a microsecond-precision timestamp rather than the event's `time.Time`. The `time.Time` caused problems because it serializes to a string with second-precision by default, so events that occur close together could end up with equal timestamps. The microsecond precision reduces the chance of incorrect sorting to a tolerable level. I did look at using CreateRevision, but I learned that CreateRevision can still be equal if the events were recorded within the same transaction. CreateRevision worked in testing, but there's a non-zero chance that it could have broken in the future. PLAT-403
7264b7e to
9b24f17
Compare
Summary
Changes the post-query pending event sort to use a microsecond-precision timestamp rather than the event's
time.Time. Thetime.Timecaused problems because it serializes to a string with second-precision by default, so events that occur close together could end up with equal timestamps. The microsecond precision reduces the chance of incorrect sorting to a tolerable level.I did look at using CreateRevision, but I learned that CreateRevision can still be equal if the events were recorded within the same transaction. CreateRevision worked in testing, but there's a non-zero chance that it could have broken in the future.
Testing
PLAT-403