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

Add support for AstraDB #40554

Conversation

michaelsembwever
Copy link

autoconfigure properties for a CassandraVectorStore cql connection to AstraDB

Provides autoconfigure properties for the additional AstraDB options to make a successful cql connection.
@michaelsembwever michaelsembwever marked this pull request as ready for review April 29, 2024 10:33
@wilkinsona
Copy link
Member

Thanks for the proposal. Please note that as this would be a new feature it cannot be added to a 3.2.x maintenance release. The earliest that it could be added would now be Spring Boot 3.4 as we've just started the RC phase for 3.3 and the feature set is frozen.

If we want a configuration property for the cloud secure connect bundle then, rather than introducing new spring.astra.* properties for it, I think it would be better for it to sit alongside existing properties that are applied to the CqlSessionBuilder that we already auto-configure. For example, spring.cassandra.keyspace-name is applied to the CqlSessionBuilder using withKeyspace. Similarly, a new spring.cassandra.cloud-secure-connect-bundle property could be applied to the CqlSessionBuilder using withCloudSecureConnectBundle.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Apr 29, 2024
@michaelsembwever
Copy link
Author

Thanks for the proposal. Please note that as this would be a new feature it cannot be added to a 3.2.x maintenance release. The earliest that it could be added would now be Spring Boot 3.4 as we've just started the RC phase for 3.3 and the feature set is frozen.

This is your decision, but it's a trivial addition… 🤷

If we want a configuration property for the cloud secure connect bundle then, rather than introducing new spring.astra.* properties for it, I think it would be better for it to sit alongside existing properties that are applied to the CqlSessionBuilder that we already auto-configure. For example, spring.cassandra.keyspace-name is applied to the CqlSessionBuilder using withKeyspace. Similarly, a new spring.cassandra.cloud-secure-connect-bundle property could be applied to the CqlSessionBuilder using withCloudSecureConnectBundle.

It can't be part of the existing Cassandra connections. This only works with AstraDB. AstraDB is CQL compatible with Cassandra, but not vice versa.

I've no objection to putting the classes into the org.springframework.boot.autoconfigure.cassandra package.
But a property spring.cassandra.cloud-secure-connect-bundle definitely implies the wrong thing.
What about spring.cassandra.astra.cloud-secure-connect-bundle instead ?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 29, 2024
@michaelsembwever michaelsembwever changed the base branch from 3.2.x to main April 29, 2024 11:58
@wilkinsona
Copy link
Member

I've no objection to putting the classes into the org.springframework.boot.autoconfigure.cassandra package.
But a property spring.cassandra.cloud-secure-connect-bundle definitely implies the wrong thing.

Generally speaking, when naming and structuring properties, we try to take our lead from the names and structure of the API that the properties are configuring. In this case the Astra-specific withCloudSecureConnectBundle method's on SessionBuilder alongside several other general Cassandra methods. Why does the API of SessionBuilder not also imply the wrong thing? If there were several Astra properties, grouping them together beneath spring.cassandra.astra would be quite compelling. With only one, deviating from the structure of the API we're using is less appealing.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 29, 2024
@michaelsembwever
Copy link
Author

michaelsembwever commented Apr 29, 2024

withCloudSecureConnectBundle only works with astra proprietary drivers.

Putting it alongside the other cassandra options would be very confusing (and likely create errors) for cassandra users.

It would be like putting new ElasticSearch properties into the OpenSearch space, that did not work with OpenSearch. Sure way to piss off the OpenSearch people.

How does spring-boot deal with different sql driver specifics, are they in one place or separated out to their dialects/DBs ? 🤷

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 29, 2024
@wilkinsona
Copy link
Member

Putting it alongside the other cassandra options would be very confusing (and likely create errors) for cassandra users.

Then why has the API of SessionBuilder/CqlSessionBuilder been modelled as it has? Is it not very confusing for the general purpose CqlSessionBuilder API to contain a method that's Astra specific? If not, what is it about Spring Boot that would make a similarly structured property name confusing?

What I'm trying to understand is why the structure of our properties should different from the structure of the API in the Cassandra Driver's Java API.

@michaelsembwever
Copy link
Author

michaelsembwever commented Apr 29, 2024

The secureConnectBundle works as part of existing cql connections and drivers, for just astra.

This information does go into the CqlSessionBuilder, but it breaks a vanilla cql connection.

What I'm trying to understand is why the structure of our properties should different from the structure of the API in the Cassandra Driver's Java API.

The java-driver was only recently donated to the ASF. When it was at DataStax then proprietary features like this could be put directly into it.

Now it's at the ASF these features will be removed over time – and then you'll see a AstraCqlSessionBuilder.

I'd envision this would eventuate as a number of things from the org.springframework.boot.autoconfigure.cassandra package being moved and/or copied to org.springframework.boot.autoconfigure.astra – and that explains my original preference to that latter package :)

Such a transition takes time, and is dependant on the community…

@philwebb
Copy link
Member

Now it's at the ASF these features will be removed over time – and then you'll see a AstraCqlSessionBuilder.

Given this fact, I think we should hold off on adding direct support to Spring Boot at this time. It looks like the existing org.springframework.boot.autoconfigure.cassandra.CqlSessionBuilderCustomizer should suffice for now.

I think we should consider adding support when the AstraCqlSessionBuilder has been added.

Thanks anyway for the PR @michaelsembwever

@philwebb philwebb closed this Apr 29, 2024
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Apr 29, 2024
@michaelsembwever
Copy link
Author

michaelsembwever commented Apr 29, 2024

I think we should hold off on adding direct support to Spring Boot at this time.

That could be years away.
And it is not a definite statement.

Currently this feature is documented here:
https://github.com/apache/cassandra-java-driver/blob/4.x/manual/cloud/README.md

For example, others may adopt this approach to DBaaS connections and then it would stay.

For a user example, take a look at this codebase in action here: https://github.com/datastax/ai-agent-java

With the application.properties it is very easy to switch from using Apache Cassandra to using AstraDB, just by adding a few properties. This interoperability is desired, and supports OSS work.

https://github.com/datastax/ai-agent-java/blob/workshop-step-1/src/main/resources/application.properties

This layout of property name spaces makes sense to me.

The last line could also easily be

spring.cassandra.astra.secure-connect-bundle=${ASTRA_SCB_PATH}

But this would be weird

spring.cassandra.secure-connect-bundle=${ASTRA_SCB_PATH}

And this would be weird, because astra users are entirely aware of the base cql standard they are working on top of…


spring.astra.localDatacenter=datacenter1

# to use AstraDB
spring.astra.username=token
spring.astra.password=${ASTRA_DB_APPLICATION_TOKEN}
spring.astra.secure-connect-bundle=${ASTRA_SCB_PATH}

Trying to grok what the spring-boot team wants here, the best I think works is…

  • putting the classes into the existing org.springframework.boot.autoconfigure.cassandra package
  • using a property prefix of either spring.cassandra.astra.secure-connect-bundle or spring.cassandra.cloud.secure-connect-bundle

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants