Skip to content

refactor(drainer): removed fred dependency from drainer - #171

Merged
jarnura merged 8 commits into
mainfrom
del-fred-dep-drainer
Dec 22, 2022
Merged

jarnura merged 8 commits into
mainfrom
del-fred-dep-drainer

Conversation

@Abhicodes-crypto

@Abhicodes-crypto Abhicodes-crypto commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

Type of Change

  • Refactoring

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

  • I formatted the code cargo +nightly fmt
  • I addressed lints thrown by cargo clippy
  • I reviewed submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@SanchithHegde SanchithHegde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've requested a couple of minor changes, other than that, looks good to me!

Comment thread crates/drainer/src/lib.rs
Comment on lines +63 to +66
let insert_op = "insert";
let update_op = "update";
let payment_intent = "payment_intent";
let payment_attempt = "payment_attempt";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Abhicodes-crypto Abhicodes-crypto Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will be removed when metrics and logs are added. @lsampras would be doing that in a separate PR.

Comment thread crates/drainer/src/utils.rs Outdated
@SanchithHegde SanchithHegde added A-framework Area: Framework S-waiting-on-author Status: This PR is incomplete or needs to address review comments C-refactor Category: Refactor labels Dec 19, 2022
@Abhicodes-crypto Abhicodes-crypto added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Dec 21, 2022

@SanchithHegde SanchithHegde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes, other than that, looks good to me!

Comment thread crates/redis_interface/src/types.rs Outdated
Comment thread crates/redis_interface/src/types.rs Outdated
@jarnura jarnura added S-awaiting-merge and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed S-awaiting-merge labels Dec 21, 2022
@jarnura
jarnura merged commit 6f847e3 into main Dec 22, 2022
@jarnura
jarnura deleted the del-fred-dep-drainer branch December 22, 2022 05:52
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-framework Area: Framework C-refactor Category: Refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants