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

Refactor account processing implementation to be more efficient #553

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

nullpointer0x00
Copy link
Contributor

Description

closes: #552


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@nullpointer0x00 nullpointer0x00 marked this pull request as ready for review October 1, 2024 21:15
@nullpointer0x00 nullpointer0x00 requested a review from a team as a code owner October 1, 2024 21:15
transaction { it.apply { this.processing = true } }
send(it.processValue)
transaction { record.apply { this.processing = true } }
accountService.updateTokenCounts(record.processValue)
Copy link

@scirner22 scirner22 Oct 1, 2024

Choose a reason for hiding this comment

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

I see now - updateTokenCounts is suspended. I think you'll want to do the following because as it stands right now the subsequent line is ran before updateTokenCounts completes, and also whether it throws an exception or not.

Suggested change
accountService.updateTokenCounts(record.processValue)
runBlocking { accountService.updateTokenCounts(record.processValue) }

Copy link

@scirner22 scirner22 Oct 1, 2024

Choose a reason for hiding this comment

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

I updated my suggested change above so that there's no blocking calls inside of the runBlocking, even this isn't idiomatic, but at least it's correct from a concurrency perspective and alleviates the need for a larger change surface area.

With this, the runBlocking on 449 can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good catch. I was hoping not to use runBlocking at all, but all the grpc client methods use suspend. I decided I would look into why this pattern is used instead of change it.

Choose a reason for hiding this comment

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

It's a great model, but requires up and down the stack to adhere to it. In this case, the jdbc driver is thread based calls so it requires constantly bridging blocking and nonblocking code.

@nullpointer0x00 nullpointer0x00 enabled auto-merge (squash) October 2, 2024 12:37
@nullpointer0x00 nullpointer0x00 merged commit db73ab3 into main Oct 2, 2024
4 checks passed
@nullpointer0x00 nullpointer0x00 deleted the nullpointer0x00/refactor-account-processing branch October 2, 2024 13:41
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.

Refactor account processing implementation to be more efficient
2 participants