-
Notifications
You must be signed in to change notification settings - Fork 21
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 SQL Server Connector #481
Conversation
mssql-jdbc requires commons-codec 1.10
401c251
to
d232ca3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't look at the product tests in depth. Also, the connector changes seem really minimal - surprising only this much is needed. Maybe we should have a generic (no code changes needed) jdbc connector already.
presto-sqlserver/pom.xml
Outdated
<groupId>com.sun.jersey</groupId> | ||
<artifactId>jersey-core</artifactId> | ||
</exclusion> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneded whitespace
pom.xml
Outdated
<excludes> | ||
<exclude>META-INF/*.SF</exclude> | ||
<exclude>META-INF/*.DSA</exclude> | ||
<exclude>META-INF/*.RSA</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an unrelated change. Also, there should be a comment why those excludes are needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will add comment and probably put them in a separate commit.
presto-sqlserver/pom.xml
Outdated
@@ -5,7 +5,7 @@ | |||
<parent> | |||
<artifactId>presto-root</artifactId> | |||
<groupId>com.facebook.presto</groupId> | |||
<version>0.160-SNAPSHOT</version> | |||
<version>0.162-SNAPSHOT-t.1-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change, probably should be in previous commit
@@ -38,7 +38,6 @@ plugin.bundles=\ | |||
../presto-local-file/pom.xml, \ | |||
../presto-mysql/pom.xml,\ | |||
../presto-postgresql/pom.xml,\ | |||
../presto-postgresql/pom.xml, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put that into separate commit.
@@ -207,7 +207,7 @@ fi | |||
trap terminate INT TERM EXIT | |||
|
|||
# start external services | |||
EXTERNAL_SERVICES="hadoop-master mysql postgres cassandra" | |||
EXTERNAL_SERVICES="hadoop-master mysql postgres cassandra sqlserver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're running quite a lot of additional services for the product tests, even if they aren't used. Does it pass on travis? We should think how to only run the necessary services. For this PR it's fine though as long as the build is stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the tests are passing on Travis. I strong believe we should have a way to run only hadoop-master + presto-master + related container, rather than starting all containers while running any given product-tests.
@@ -50,7 +50,7 @@ | |||
<dep.packaging.version>${dep.airlift.version}</dep.packaging.version> | |||
<dep.slice.version>0.27</dep.slice.version> | |||
<dep.aws-sdk.version>1.11.30</dep.aws-sdk.version> | |||
<dep.tempto.version>1.22</dep.tempto.version> | |||
<dep.tempto.version>1.23</dep.tempto.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect a separate 'update tempto' commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already updated tempto to 1.23 on prestodb/master. This will be removed while merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be separate commit anyway though. When on master it would it would be empty commit.
Someone having more experience with connectors should take a look. |
Took another look at the product tests and they seem far simpler than what we have for cassandra ATM. We wanted those to be similar. Also, cassandra uses code-defined product tests while sqlserver - convention based. I'd opt for having code-defined product tests reused for testing both connectors. |
@ArturGajowy - Yes, java based based are on their way. The convention based tests are only for testing basic SQL queries. |
presto-sqlserver/pom.xml
Outdated
<parent> | ||
<artifactId>presto-root</artifactId> | ||
<groupId>com.facebook.presto</groupId> | ||
<version>0.160-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's against sprint-47
so it should be 0.162-SNAPSHOT-t.1-SNAPSHOT
.
presto-sqlserver/pom.xml
Outdated
<scope>provided</scope> | ||
</dependency> | ||
|
||
<!-- Presto SPI --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary.
@@ -50,7 +50,7 @@ | |||
<dep.packaging.version>${dep.airlift.version}</dep.packaging.version> | |||
<dep.slice.version>0.27</dep.slice.version> | |||
<dep.aws-sdk.version>1.11.30</dep.aws-sdk.version> | |||
<dep.tempto.version>1.22</dep.tempto.version> | |||
<dep.tempto.version>1.23</dep.tempto.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be separate commit anyway though. When on master it would it would be empty commit.
hostname: sqlserver | ||
image: 'microsoft/mssql-server-linux' | ||
ports: | ||
- '1433:1433' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use not default port if possible, similarly to what we have for postgres and mysql.
This is to prevent clash with dev env while working with another instance of MsSQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike MySQL and PostgreSQL, we can't pass port as the parameter. We will have to create a custom docker image on top of sql-server. As discussed, for now I'll just run on 1433
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created this issue to track this for SQL Server in Docker microsoft/mssql-docker#21
ports: | ||
- '1433:1433' | ||
environment: | ||
ACCEPT_EULA: Y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work with our apache-licensed software and our automation? Can we legally use it on Travis?
This should be AT LEAST mentioned in README.
@@ -0,0 +1,4 @@ | |||
connector.name=sqlserver | |||
connection-url=jdbc:sqlserver://sqlserver | |||
connection-user=sa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why sa
? Why don't we use the same credentials as for other JDBC connectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have that provision. Microsoft is working on that
I'll ignore it for this PR
jdbc_driver_class: com.microsoft.sqlserver.jdbc.SQLServerDriver | ||
jdbc_url: jdbc:sqlserver://sqlserver | ||
jdbc_user: sa | ||
jdbc_password: SQLServerPass1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -102,7 +102,7 @@ function stop_docker_compose_containers() { | |||
stop_application_runner_containers ${ENVIRONMENT} | |||
|
|||
# stop containers started with "up", removing their volumes | |||
environment_compose down -v | |||
environment_compose down -v || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is needed? Is there a legitimate reason to accept not gracefully shut down sql server container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say yes - we had cases where all the tests passed, yet docker-compose got a timeout when trying to communicate with the sqlserver container. Since the problem is hard to reproduce locally, we've no idea how to approach a fix, and it does not affect the tests themselves - I'd stick with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should have a comment next to that line to make it clear it shouldn't be that way but we cannot fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed - 7929af9
{ | ||
super(connectorId, config, "\"", new SQLServerDriver()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlServer supports transactional DDLs. Why don't you use it instead of relying on default implementation of BaseJdbcClient
providing non-ACID compliant DDLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't use transactions because the statements are executed from multiple connections.
d232ca3
to
7929af9
Compare
I fixed an issue with create table for SQL Server where the syntax used in the BaseJdbcClient wasn't supported in SQL Server. I'll talk to Sanjay about product tests. |
public void commitCreateTable(JdbcOutputTableHandle handle) | ||
{ | ||
StringBuilder sql = new StringBuilder() | ||
.append("sp_rename ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
Why not overriding beginCreateTable
, then having commitCreateTable
straightforward - simply calling commit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do that because it's done in two different connections, so the transaction's gone (you get an error "The commit transaction request has no corresponding begin transaction"). also in the interim you could have an insert into, which is done separately, but also needs to be a part of the same transaction.
After offline discussion, when it was confirmed by @rschlussel that:
I approve these changes. |
https://prestodb.io/docs/current/connector/mysql.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mod small comment
@@ -616,6 +623,12 @@ | |||
</dependency> | |||
|
|||
<dependency> | |||
<groupId>com.microsoft.sqlserver</groupId> | |||
<artifactId>mssql-jdbc</artifactId> | |||
<version>6.1.0.jre8</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure we know and document what versions of MS SQL Server this driver supports
I'll close this PR and make the docs changes in a separate commit |
Presto-product-tests has a dependency on SQL Server (mssql-jdbc) which is a signed jar. Its invalidated when we repackage it into an uber jar. Hence to avoid Error: A JNI error has occurred, please check your installation and try again Exception in thread "main" java.lang.SecurityException: Invalid signature file digest for Manifest main attributes We remove signed information from META-INF
ALTER TABLE cannot be used to rename a table in SQL Server, instead sp_rename must be used. Override the commitCreatetable method from BaseJdbcClient to use the correct SQL Server syntax.
19b2b7b
to
627313b
Compare
Initial commit with SQL Server connector. I am working on adding additional tests.
The tests run SQL Server in docker.