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: rewrite on top of Java driver 4.5.0 #2182

Merged
merged 4 commits into from
Mar 11, 2020

Conversation

ennru
Copy link
Member

@ennru ennru commented Mar 10, 2020

Replace the Alpakka Cassandra connector with a rewrite based on the Datastax Java Driver 4.6.0.

This new implementation is the basis for Cassandra connectivity in Akka Persistence Cassandra 1.0.

This work has been prepared in the Akka Persistence Cassandra repository.

This implementation is compatible with Akka 2.5 and 2.6, and published for Scala versions 2.12 and 2.13.

References #1213, #1647

@ennru ennru changed the title Cassandra: rewrite on to of Java driver 4.6.0 Cassandra: rewrite on top of Java driver 4.6.0 Mar 10, 2020
Copy link
Member

@seglo seglo left a comment

Choose a reason for hiding this comment

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

Very nice and complete and implementation. I left a few suggestions and questions.

Comment on lines +31 to +32
* The `init` hook is called before the underlying session is used by other methods,
* so it can be used for things like creating the keyspace and tables.
Copy link
Member

Choose a reason for hiding this comment

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

Great idea. This would be helpful in some of the other connectors, like JMS.

Comment on lines 51 to 52
* A flow writing to Cassandra for every stream element, passing context along.
* The element and context are emitted unchanged.
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth mentioning that the context in this case is the element being persisted.

I like this use of withContext. Is there any reason why we can't always use it instead of providing non-context factory method too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be worth mentioning that the context in this case is the element being persisted.

I'm not sure what you mean. The context can be anything.

Is there any reason why we can't always use it instead of providing non-context factory method too?

That would require to have tuples as elements.

Copy link
Member

@seglo seglo Mar 11, 2020

Choose a reason for hiding this comment

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

I'm not sure what you mean. The context can be anything.

It wasn't clear to me immediately if the context would be the input element, or some other thing (i.e. Cassandra persistence metadata maybe?)

That would require to have tuples as elements.

I was suggesting only providing a flow as a FlowWithContext. If users wanted a normal flow they could drop down to it with asSource. Maybe that's too much though.

def connect(system: ActorSystem, config: Config)(implicit ec: ExecutionContext): Future[CqlSession] = {
readNodes(config)(system, ec).flatMap { contactPoints =>
val driverConfigWithContactPoints = ConfigFactory.parseString(s"""
basic.contact-points = [${contactPoints.mkString("\"", "\", \"", "\"")}]
Copy link
Member

Choose a reason for hiding this comment

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

This threw me through a bit of a loop.

Suggested change
basic.contact-points = [${contactPoints.mkString("\"", "\", \"", "\"")}]
basic.contact-points = [${contactPoints.mkString(start = "\"", sep = "\", \"", end = "\"")}]

Copy link
Member Author

Choose a reason for hiding this comment

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

What kind of loop? I find the compact notation better.

Copy link
Member

Choose a reason for hiding this comment

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

A cognitive loop ;) I couldn't figure out if I was looking at one string or 3 until I made the params explicit.

docker-compose.yml Outdated Show resolved Hide resolved
docs/src/main/paradox/cassandra.md Outdated Show resolved Hide resolved
docs/src/main/paradox/cassandra.md Outdated Show resolved Hide resolved
docs/src/main/paradox/cassandra.md Outdated Show resolved Hide resolved
docs/src/main/paradox/cassandra.md Outdated Show resolved Hide resolved
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM, I have reviewed and contributed in APC

@ennru ennru merged commit 13f234c into akka:master Mar 11, 2020
@ennru ennru deleted the cassandra-session-move branch March 11, 2020 07:56
@ennru ennru changed the title Cassandra: rewrite on top of Java driver 4.6.0 Cassandra: rewrite on top of Java driver 4.5.0 Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants