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

streaming functions for doobie and slick with tests #108

Open
wants to merge 93 commits into
base: master
Choose a base branch
from

Conversation

salamonpavel
Copy link
Contributor

@salamonpavel salamonpavel commented Dec 22, 2023

Added support for streaming for both Slick and Doobie. Since Slick works with Futures but for streaming it relies on IOs or other effect types there was a need to abstract the streaming operation in a new DBStreamingEngine.

There are following modules:

  • core - provides core non-streaming functionality of fa-db
  • streaming - provides streaming functionality of fa-db
  • slick - provides slick version of fa-db implementation for non-streaming functions
  • slick-streaming - provides slick version of fa-db implementation for streaming functions
  • doobie - provides doobie version of fa-db implementation for both streaming and non-streaming functions

Closes #107

Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

Found a few tiny things and Jacoco coverage action is still failing (not on insufficient coverage though), so please take a look. Otherwise LGTM.

I pulled it, reviewed, compiled and ran tests.

Base automatically changed from feature/doobie-with-status to master January 3, 2024 14:46
# Conflicts:
#	README.md
#	build.sbt
#	core/src/main/scala/za/co/absa/fadb/DBFunction.scala
#	core/src/main/scala/za/co/absa/fadb/DBSchema.scala
#	doobie/src/main/scala/za/co/absa/fadb/doobie/DoobieFunction.scala
#	project/Dependencies.scala
#	slick/src/main/scala/za/co/absa/fadb/slick/SlickFunction.scala
@salamonpavel salamonpavel added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jan 3, 2024
Copy link

github-actions bot commented Jan 3, 2024

JaCoCo code coverage report - scala 2.12.17

Total Project Coverage 58.19% 🍏
Module Coverage
fa-db:core Jacoco Report - scala:2.12.17 61.79%
fa-db:streaming Jacoco Report - scala:2.12.17 0%
Files
Module File Coverage [48.91%]
fa-db:core Jacoco Report - scala:2.12.17 NamingConvention.scala 100% 🍏
StandardStatusHandling.scala 100% 🍏
DBSchema.scala 93.88% 🍏
ExplicitNamingRequired.scala 87.5% 🍏
LettersCase.scala 85.29% 🍏
SnakeCaseNaming.scala 84.29% 🍏
AsIsNaming.scala 40.63%
DBEngine.scala 20%
DBFunction.scala 6.83%
fa-db:streaming Jacoco Report - scala:2.12.17 DBStreamingFunction.scala 0%
DBStreamingEngine.scala 0%

Copy link

github-actions bot commented Jan 3, 2024

JaCoCo code coverage report - scala 2.13.12

Total Project Coverage 59.46% 🍏
Module Coverage
fa-db:core Jacoco Report - scala:2.13.12 63.49%
fa-db:streaming Jacoco Report - scala:2.13.12 0%
Files
Module File Coverage [49.53%]
fa-db:core Jacoco Report - scala:2.13.12 LettersCase.scala 100% 🍏
ExplicitNamingRequired.scala 100% 🍏
NamingConvention.scala 100% 🍏
StandardStatusHandling.scala 100% 🍏
DBSchema.scala 93.88% 🍏
SnakeCaseNaming.scala 89.29% 🍏
AsIsNaming.scala 72.22%
DBEngine.scala 20%
DBFunction.scala 7.11%
fa-db:streaming Jacoco Report - scala:2.13.12 DBStreamingFunction.scala 0%
DBStreamingEngine.scala 0%

@salamonpavel
Copy link
Contributor Author

salamonpavel commented Jan 4, 2024

Found a few tiny things and Jacoco coverage action is still failing (not on insufficient coverage though), so please take a look. Otherwise LGTM.

I pulled it, reviewed, compiled and ran tests.

Fixed this. The problem was in node version of Github's updated runner. Solved by an update of the jacoco github action version.

@salamonpavel
Copy link
Contributor Author

salamonpavel commented Jan 4, 2024

@lsulak @jakipatryk Thanks for the initial review here and on the calls we had yesterday. Based on your comments I have made the streaming part for Slick as a separate package since additional dependencies are needed and we don't want to be importing transitive dependencies into our projects when streaming is not used. At the moment there is core module for non-streaming, streaming module for streaming, slick module for slick non-streaming, slick-streaming module for slick streaming and doobie module. Doobie module provides both non-streaming and streaming functions since Doobie already bring all the dependencis needed (works with fs2 streams under the hood) so there would be no benefit in splitting this into separate modules.

@salamonpavel salamonpavel removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jan 4, 2024
Copy link
Contributor

@miroslavpojer miroslavpojer left a comment

Choose a reason for hiding this comment

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

  • pulled
  • built
  • test run
  • jacoco parts - code review

${{ github.workspace }}/slick/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
${{ github.workspace }}/slick-streaming/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
${{ github.workspace }}/doobie/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not agree with removing the code coverage fail-check logic - see the comment on the end of the file.
As I know there is no hard merge block enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@miroslavpojer I am not satisfied with this situation either. Unfortunately I couldn't perform merge of previous pull request with this check in place. Let's work together to find a good solution so we can perform tests against a database both locally and within a Github pipeline; for instance with a dockerized Postgres instance.

@@ -93,7 +97,9 @@ jobs:
with:
paths: >
${{ github.workspace }}/core/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml,
${{ github.workspace }}/streaming/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml,
${{ github.workspace }}/slick/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
${{ github.workspace }}/slick/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
${{ github.workspace }}/slick/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml,

${{ github.workspace }}/slick/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
${{ github.workspace }}/slick-streaming/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
${{ github.workspace }}/slick-streaming/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml
${{ github.workspace }}/slick-streaming/target/scala-${{ matrix.scalaShort }}/jacoco/report/jacoco.xml,

Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

LGTM, just read the code

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.

Fetching database records with streaming approach
3 participants