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 Yugabyte connector #5708

Closed
wants to merge 1 commit into from
Closed

Add Yugabyte connector #5708

wants to merge 1 commit into from

Conversation

tedyu
Copy link

@tedyu tedyu commented Oct 27, 2020

This creates presto-yugabyte module

Part of #5352

  • Add tests
  • Add yugabyte.rst to presto-docs/src/main/sphinx/connector
  • Update presto-docs/src/main/sphinx/connector.rst

presto-yugabyte/pom.xml Outdated Show resolved Hide resolved
@findepi findepi added the needs-docs This pull request requires changes to the documentation label Oct 28, 2020
@findepi
Copy link
Member

findepi commented Oct 28, 2020

@tedyu Would it be possible to add a TestYugabyteIntegrationSmokeTest similar to TestCassandraIntegrationSmokeTest?
You will need

  • TestingYugabyteServer
  • YugabyteQueryRunner that takes the TestingYugabyteServer and produces DistributedQueryRunner
    • see CassandraQueryRunner for example

presto-server/src/main/provisio/presto.xml Show resolved Hide resolved
presto-yugabyte/pom.xml Outdated Show resolved Hide resolved
presto-yugabyte/pom.xml Outdated Show resolved Hide resolved
@tedyu
Copy link
Author

tedyu commented Nov 12, 2020

When building the module with dependency on 3.2.0-yb-19-1 of yugabyte-driver, I got the following warning:

[WARNING] Unused declared dependencies found:
[WARNING]    io.prestosql.yugabyte:yugabyte-driver:jar:3.2.0-yb-19-1:compile

@findepi
Copy link
Member

findepi commented Nov 12, 2020

You should be able to use runtime scope for this.

presto-yugabyte/pom.xml Outdated Show resolved Hide resolved
@findepi
Copy link
Member

findepi commented Nov 13, 2020

@tedyu can you make sure the build is green?

@tedyu
Copy link
Author

tedyu commented Nov 16, 2020

@findepi
The build is green.

Please take another look.

@tedyu
Copy link
Author

tedyu commented Nov 17, 2020

@findepi
Is there anything else I should do ?

@ebyhr
Copy link
Member

ebyhr commented Nov 17, 2020

@tedyu Could you add tests as suggested in #5708 (comment)?

@tedyu
Copy link
Author

tedyu commented Nov 18, 2020

I have other priorities at day job.
It seems the integration test would need non-trivial effort.

Can we do that later ?

@tedyu
Copy link
Author

tedyu commented Nov 18, 2020

ping @findepi to see if this can go in.

thanks

presto-yugabyte/pom.xml Outdated Show resolved Hide resolved
@tedyu
Copy link
Author

tedyu commented Nov 19, 2020

@findepi
Test dependencies have been removed.

@findepi
Copy link
Member

findepi commented Nov 19, 2020

@tedyu thanks for removing some test dependencies. Can you remove those remaining ones as well please?

@tedyu
Copy link
Author

tedyu commented Nov 19, 2020

@findepi
Remaining ones have been removed.

findepi
findepi previously approved these changes Nov 19, 2020
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

@ebyhr PTAL

@ebyhr
Copy link
Member

ebyhr commented Nov 20, 2020

@tedyu I'm testing this PR locally, but it throws NPE during servrer initialization. Is this PR working in your environment?

connector.name=yugabyte
cassandra.contact-points=yugabyte
cassandra.native-protocol-port=9042
1) Error in custom provider, java.lang.NullPointerException
  at io.prestosql.plugin.cassandra.CassandraClientModule.createCassandraSession(CassandraClientModule.java:92)
  while locating io.prestosql.plugin.cassandra.CassandraSession
Caused by: java.lang.NullPointerException
	at com.datastax.driver.core.Cluster$Manager.<init>(Cluster.java:1408)
	at com.datastax.driver.core.Cluster$Manager.<init>(Cluster.java:1355)
	at com.datastax.driver.core.Cluster.<init>(Cluster.java:125)
	at com.datastax.driver.core.Cluster.<init>(Cluster.java:98)
	at com.datastax.driver.core.DelegatingCluster.<init>(DelegatingCluster.java:45)
	at io.prestosql.plugin.cassandra.ReopeningCluster.<init>(ReopeningCluster.java:44)

@findepi
Copy link
Member

findepi commented Nov 20, 2020

@ebyhr thanks for catching this

this means it's not so simple. we should add

  1. a test similar to TestPostgreSqlPlugin (for both Yugabyte and Cassandra ideally)
  2. a test similar to TestCassandraIntegrationSmokeTest
  3. a product test, see SinglenodeCassandra for a starting point

@tedyu
Copy link
Author

tedyu commented Nov 21, 2020

With upcoming Holidays, response may be slow.

@ebyhr
Copy link
Member

ebyhr commented Feb 15, 2021

@tedyu Are you still working on this? Do you mind if someone take over this?

@tedyu
Copy link
Author

tedyu commented Feb 15, 2021

Taking over for creating presto-yugabyte module ?

Sure.

@findepi findepi changed the title Create presto-yugabyte module Add Yugabyte connector Feb 15, 2021
@ebyhr
Copy link
Member

ebyhr commented Jul 24, 2021

I'm trying the Yugabyte driver locally and realized a new schema or table still doesn't appear when listing in Trino. It seems the root cause is Session is reused in CassandraSession class by memoize(cluster::connect) and also ReopeningCluster needs fix.

I would check if we can just fix in Cassandra connector.

@ebyhr
Copy link
Member

ebyhr commented Jul 24, 2021

About bootstrap issue #5708 (comment), DelegatingCluster passes null as Configuration at
DelegatingCluster.java#L49 and it causes NPE at Cluster.java#L1620

yugabyte/cassandra-java-driver#8

@ebyhr
Copy link
Member

ebyhr commented Aug 3, 2021

As far as I know, YCQL and YSQL are independent (= tables created in YCQL doesn't appear in YSQL). We may want to avoid just yugabyte as a connector name and rename to yugabyte-cassandra, yugabyte-cql or something.

@mosabua
Copy link
Member

mosabua commented Oct 20, 2022

👋 @tedyu @ebyhr - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

@colebow colebow closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed needs-docs This pull request requires changes to the documentation
Development

Successfully merging this pull request may close these issues.

6 participants