refactor(drainer): removed fred dependency from drainer - #171
Merged
Merged
Conversation
SanchithHegde
requested changes
Dec 19, 2022
SanchithHegde
left a comment
Member
There was a problem hiding this comment.
I've requested a couple of minor changes, other than that, looks good to me!
Comment on lines
+63
to
+66
| let insert_op = "insert"; | ||
| let update_op = "update"; | ||
| let payment_intent = "payment_intent"; | ||
| let payment_attempt = "payment_attempt"; |
Member
There was a problem hiding this comment.
Maybe use const for all these string literals?
And maybe rename them along the lines of insert_operation, payment_intent_type_name (or payment_intent_table_name) instead?
Contributor
Author
There was a problem hiding this comment.
These will be removed when metrics and logs are added. @lsampras would be doing that in a separate PR.
SanchithHegde
requested changes
Dec 21, 2022
SanchithHegde
left a comment
Member
There was a problem hiding this comment.
Minor changes, other than that, looks good to me!
SanchithHegde
approved these changes
Dec 21, 2022
jarnura
approved these changes
Dec 21, 2022
maverox
added a commit
that referenced
this pull request
Sep 18, 2026
… collections facade TEMPORARY PIN. The nine deja revisions move from main `8d87c19` to `159aadc`, the head of deja's `work/teardown-safe-correlation-reader` (PR #171). Every manifest says so, because a bare revision looks exactly like a released one. This must be bumped back to a main commit once #171 merges, and that bump is a precondition for review rather than a follow-up to it. A revision rather than `branch = "..."`. A branch pin re-resolves on any lock refresh, so the deja underneath would move without a manifest edit — and deja records its own sha into every tape it writes, which makes a moving pin a moving experiment. The branch name rides in the comment, where it informs without deciding what gets built. WHY IT CANNOT WAIT FOR THE MERGE. `common_utils::collections` gives `HashMap`/`HashSet` a correlation-derived hash seed, so a set's iteration order is the same in a replay as in the recording. It does not compile against deja main at all: `Default for CorrelationHasher` runs on every `HashMap::new()`, destructors included, and `current_correlation_id` reads its thread-local with a plain `with`/`borrow`, which ABORTS the process there rather than failing a request. #171 adds the fallible twin the facade calls. Pinning the branch is what lets this be built and replayed now instead of after two merges. WHAT IT DOES NOT DO. Nothing calls the facade. It is the mechanism, not the adoption — the constructors exist and no collection uses them yet. The target is `payment_methods_enabled`, built at `crates/router/src/core/payment_methods/cards.rs:5215` by iterating nested HashMaps into Vecs; ordering there is 26 of 27 blocking body mismatches on the run we care about. That construction path is verified. The causal link to those 26 is strongly indicated and NOT proven against a failing scorecard, which is exactly what a build carrying this is for. CHECKED. `common_utils` at --features deja and `router` at --features v1,deja,release, both clean. The common_utils green was verified capable of failing first: the seeded hasher sits behind `#[cfg(feature = "deja")]`, so a run without it compiles none of the file and reports success anyway. A `compile_error!` inside `Default for CorrelationHasher` fired under this feature set.
5 tasks
maverox
added a commit
that referenced
this pull request
Sep 21, 2026
… collections facade TEMPORARY PIN. The nine deja revisions move from main `8d87c19` to `159aadc`, the head of deja's `work/teardown-safe-correlation-reader` (PR #171). Every manifest says so, because a bare revision looks exactly like a released one. This must be bumped back to a main commit once #171 merges, and that bump is a precondition for review rather than a follow-up to it. A revision rather than `branch = "..."`. A branch pin re-resolves on any lock refresh, so the deja underneath would move without a manifest edit — and deja records its own sha into every tape it writes, which makes a moving pin a moving experiment. The branch name rides in the comment, where it informs without deciding what gets built. WHY IT CANNOT WAIT FOR THE MERGE. `common_utils::collections` gives `HashMap`/`HashSet` a correlation-derived hash seed, so a set's iteration order is the same in a replay as in the recording. It does not compile against deja main at all: `Default for CorrelationHasher` runs on every `HashMap::new()`, destructors included, and `current_correlation_id` reads its thread-local with a plain `with`/`borrow`, which ABORTS the process there rather than failing a request. #171 adds the fallible twin the facade calls. Pinning the branch is what lets this be built and replayed now instead of after two merges. WHAT IT DOES NOT DO. Nothing calls the facade. It is the mechanism, not the adoption — the constructors exist and no collection uses them yet. The target is `payment_methods_enabled`, built at `crates/router/src/core/payment_methods/cards.rs:5215` by iterating nested HashMaps into Vecs; ordering there is 26 of 27 blocking body mismatches on the run we care about. That construction path is verified. The causal link to those 26 is strongly indicated and NOT proven against a failing scorecard, which is exactly what a build carrying this is for. CHECKED. `common_utils` at --features deja and `router` at --features v1,deja,release, both clean. The common_utils green was verified capable of failing first: the seeded hasher sits behind `#[cfg(feature = "deja")]`, so a run without it compiles none of the file and reports success anyway. A `compile_error!` inside `Default for CorrelationHasher` fired under this feature set.
maverox
added a commit
that referenced
this pull request
Sep 22, 2026
TEMPORARY PIN. The nine deja revisions move from main `8d87c19` to `159aadc`, the head of deja's `work/teardown-safe-correlation-reader` (PR #171). Every manifest says so, because a bare revision looks exactly like a released one. This must be bumped back to a main commit once #171 merges, and that bump is a precondition for review rather than a follow-up to it. A revision rather than `branch = "..."`. A branch pin re-resolves on any lock refresh, so the deja underneath would move without a manifest edit — and deja records its own sha into every tape it writes, which makes a moving pin a moving experiment. The branch name rides in the comment, where it informs without deciding what gets built. WHY. #171 adds `try_current_correlation_id`, the fallible twin of `current_correlation_id`. The collections facade, raised as its own change on top of this one, calls it from `Default for CorrelationHasher`, which runs on every `HashMap::new()`, destructors included; the plain reader reads its thread-local with `with`/`borrow` and aborts the process there rather than failing a request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
Removed fred dependency from drainer
Motivation and Context
Removed direct fred dependency to make the drainer, redis-client lib agnostic.
How did you test it?
Tested on local
Checklist
cargo +nightly fmtcargo clippy