Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove dependency of postgres trigger for broadcaster #11109

Merged
merged 13 commits into from
Oct 31, 2023

Conversation

poopoothegorilla
Copy link
Contributor

@poopoothegorilla poopoothegorilla commented Oct 27, 2023

Notes:

  • remove postgres dependency from common broadcaster
  • add migration to remove postgres trigger which fires on tx inserts
  • replace pkgerrors where possible with fmt.Errorf. The overall codebase would like to remove reliance on pkerrors pkg
  • switch from using the postgres trigger for new TxInserts and use the force Trigger Method after Transaction Creations instead
  • removing the postgres dependency is required for future in memory caching layer work
  • The postgres notifications would trigger ONLY on INSERTS... this change will be Trigger on successful txStore.CreateTransaction calls, which could include non inserts. The only impact this will have is potentially more checks for new Transactions.

@poopoothegorilla poopoothegorilla requested a review from a team as a code owner October 27, 2023 15:25
@github-actions
Copy link
Contributor

I see that you haven't updated any README files. Would it make sense to do so?

common/txmgr/txmgr.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrick-dowell patrick-dowell left a comment

Choose a reason for hiding this comment

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

It's awesome this PR cleans up so much code!

I'm not sure exactly what's going on with the switch from pkg/errors.wrap to fmt.Errorf. I assume it's related to the overall change in error changing cause by the removal of the trigger dependency, but I don't entirely understand it.

common/txmgr/txmgr.go Show resolved Hide resolved
@@ -223,8 +222,10 @@ func (b *Txm[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) Reset(addr
func (b *Txm[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) abandon(addr ADDR) (err error) {
ctx, cancel := utils.StopChan(b.chStop).NewCtx()
defer cancel()
err = b.txStore.Abandon(ctx, b.chainID, addr)
return pkgerrors.Wrapf(err, "abandon failed to update txes for key %s", addr.String())
if err = b.txStore.Abandon(ctx, b.chainID, addr); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it a common go pattern to do the assignment and comparison this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I typically am a fan of doing it this way to prevent accidental overshadowing of the error... i have witnessed bugs in the past where the error does not get fully handled and gets overwritten accidentally. scoping it this way reduces the chances of that happening slightly

common/txmgr/txmgr.go Outdated Show resolved Hide resolved
Copy link
Contributor

@amit-momin amit-momin left a comment

Choose a reason for hiding this comment

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

LGTM now!

Copy link
Contributor

@prashantkumar1982 prashantkumar1982 left a comment

Choose a reason for hiding this comment

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

This is so awesome!

}

// Trigger the Broadcaster to check for new transaction
b.broadcaster.Trigger(txRequest.FromAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

love to see how simple this now is!

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 57.7% 57.7% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

@prashantkumar1982 prashantkumar1982 added this pull request to the merge queue Oct 31, 2023
Merged via the queue into develop with commit 8c96682 Oct 31, 2023
83 of 84 checks passed
@prashantkumar1982 prashantkumar1982 deleted the jtw/replace-postgres-triggers-txm branch October 31, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants