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

cassandra-alpakka #2365

Merged
merged 2 commits into from
Jan 14, 2022
Merged

cassandra-alpakka #2365

merged 2 commits into from
Jan 14, 2022

Conversation

justcoon
Copy link
Contributor

@justcoon justcoon commented Jan 3, 2022

Problem

Context with alpakka cassandra session
https://doc.akka.io/docs/alpakka/current/cassandra.html

Solution

added new module with CassandraAlpakkaContext

Notes

open problems:

  • keyspace configuration
  • Await in case of underlying CqlSession - used for UDT

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@deusaquilus
Copy link
Collaborator

Hey @justcoon. I think we should take out the batching implementation since it's just a sequence of individual actions (just change the function to fail and say it's not supported). At some point we are going to want to implement actual cassandra native-batching in all the contexts but I don't know what that's supposed to look like yet.
Let's get a build of this working and I can approve. Please let me know if you need anything from my end.

@justcoon
Copy link
Contributor Author

justcoon commented Jan 13, 2022

@deusaquilus

I assume you mean CassandraAlpakkaContext.executeBatchAction

in general other CassandraContext-s got this function, so personally I think it will be better, if this context will also support that

if implementation of batching will be changed later, it will have to be changed also in other CassandraContext-s

@justcoon justcoon closed this Jan 13, 2022
@justcoon justcoon reopened this Jan 13, 2022
@justcoon
Copy link
Contributor Author

Hello @deusaquilus,

if I do

  def executeBatchAction(
    groups: List[BatchGroup]
  )(info: ExecutionInfo, dc: Runner)(implicit executionContext: ExecutionContext): Result[RunBatchActionResult] = {
    Future.failed(new UnsupportedOperationException("Batch action is not supported"))
  }

current tests (which was made in similar way, like in quill-cassandra module) will also fail

[info] PeopleCassandraSpec:
[info] io.getquill.context.cassandra.alpakka.PeopleCassandraSpec *** ABORTED ***
[info]   java.lang.UnsupportedOperationException: Batch action is not supported
[info]   at io.getquill.CassandraAlpakkaContext.executeBatchAction(CassandraAlpakkaContext.scala:90)
[info]   at io.getquill.context.cassandra.alpakka.PeopleCassandraSpec.beforeAll(PeopleCassandraSpec.scala:25)

of course, it is expected ..., (test will have to be also fixed with this change).

Please, can you confirm, that now you prefer to have the operation as unsupported? Or we will support that like in other CassandraContext-s.

Thank you

@deusaquilus
Copy link
Collaborator

Please, can you confirm, that now you prefer to have the operation as unsupported? Or we will support that like in other CassandraContext-s.

If the others already do that you can leave it. This looks complete.

def executeBatchAction(
groups: List[BatchGroup]
)(info: ExecutionInfo, dc: Runner)(implicit executionContext: ExecutionContext): Result[RunBatchActionResult] = {
Future
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the batching implementation is just sequencing a single operation I don't think it's worth implementing. I think Cassandra supports batching natively which we should try to use sometime in the future.

import io.getquill.context.cassandra.udt.UdtSpec
import io.getquill.Udt

class UdtEncodingSessionContextSpec extends UdtSpec with CassandraAlpakkaSpec {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great to see that this supports UDTs!

@deusaquilus deusaquilus merged commit b217f21 into zio:master Jan 14, 2022
@deusaquilus
Copy link
Collaborator

Thanks for the contribution @justcoon! Let me know if you're interested in also adding this into ProtoQuill.

@deusaquilus deusaquilus changed the title WIP cassandra-alpakka cassandra-alpakka Jan 19, 2022
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.

2 participants