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

Support for MS SQL #731

Merged
merged 9 commits into from
May 22, 2017
Merged

Support for MS SQL #731

merged 9 commits into from
May 22, 2017

Conversation

juliano
Copy link
Collaborator

@juliano juliano commented Mar 26, 2017

Fixes #451

Problem

Quill doesn't support MS SQL Server. This PR adds this support.

Solution

Implemented Sql Server context and dialect

Notes

As suggested by @gustavoamigo I put this code in a different project in order to not mess up with the current build or make it slower, once that I need a different docker image to do the setup. To set up the entire environment just run:

docker-compose stop && docker-compose rm && docker-compose run --rm setup && docker-compose run --rm setup-sqlserver

I am not entirely sure if this is the way to go, any suggestion here would be great.

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

@mxl
Copy link
Contributor

mxl commented Mar 26, 2017

Nice! But should not we keep all jdbc related stuff in quill-jdbc project? (See #665)

@mosyp
Copy link
Collaborator

mosyp commented Mar 27, 2017

@getquill/maintainers We should definitely split jdbc module to different artifacts, as with async modules. Ideally, move quill-jdbc drivers/implementations to different repository

@juliano
Copy link
Collaborator Author

juliano commented Mar 28, 2017

So, do we have a consensus? Can we go with a different project?

@fwbrasil
Copy link
Collaborator

fwbrasil commented Apr 1, 2017

@juliano Sorry for the late feeback. I'd prefer to keep all jdbc integration under quill-jdbc since Quill doesn't depend directly on any specific driver, except for tests, and the jdbc api is pretty stable.

@juliano
Copy link
Collaborator Author

juliano commented Apr 3, 2017

@fwbrasil sounds good to me. I'll change the test containers and put everything under quill-jdbc

@fwbrasil
Copy link
Collaborator

@juliano any updates on the changes?

@juliano
Copy link
Collaborator Author

juliano commented Apr 13, 2017

@fwbrasil not yet because I am on holidays, I will be back in 10 days :)

@fwbrasil
Copy link
Collaborator

@juliano enjoy! :)

@@ -0,0 +1,11 @@
FROM phusion/baseimage
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use debian:jessie image because all other containers are using it or better move mssql setup to setup container. Unfortunately official mssql container is available only as Ubuntu and RHEL based so this will be the only exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update that! 👍

@mxl
Copy link
Contributor

mxl commented May 18, 2017

@juliano We are solved issue with long running build by parallelizing Scala 2.11 and 2.12 builds. Also there are other possibilities to parallelize build, e.g. to build each module in separate job.
I thought about splitting this repository into separate repositories for each module as @mentegy suggested but this will create many maintaining problems and mess which we will not able to handle.

@kentongray
Copy link

WOW! @juliano you are my hero :godmode:

@juliano
Copy link
Collaborator Author

juliano commented May 19, 2017

@kentongray I am glad to help! :)

@mxl it's almost there, but it seems I need some help :/

@mxl
Copy link
Contributor

mxl commented May 19, 2017

@juliano I'm here or ping me in Gitter.

@mxl mxl force-pushed the sqlserver branch 2 times, most recently from 4b73e79 to 3eca2cd Compare May 20, 2017 15:52
@mxl
Copy link
Contributor

mxl commented May 20, 2017

@juliano Problem solved in #784

@juliano juliano force-pushed the sqlserver branch 2 times, most recently from ae4c835 to 3a2833d Compare May 21, 2017 09:45
@juliano
Copy link
Collaborator Author

juliano commented May 22, 2017

@mxl it seems it didn't worked :/
Can you please check if the problem is still the same?

Copy link
Contributor

@mxl mxl left a comment

Choose a reason for hiding this comment

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

@juliano For some reason it fails when connecting to MS SQL Server because database was damaged(?):

Sql Server ready
Msg 922, Level 14, State 1, Server 8eedbd6c1d56, Line 1
Database 'model' is being recovered. Waiting until recovery is finished.
Sqlcmd: Error: Microsoft ODBC Driver 13 for SQL Server : Cannot open database "quill_test" requested by the login. The login failed..
Sqlcmd: Error: Microsoft ODBC Driver 13 for SQL Server : Login failed for user 'sa'..


RUN dpkg-reconfigure -f noninteractive tzdata && \
sed -i -e 's/# en_US.UTF-8 UTF-8/en_US.UTF-8 UTF-8/' /etc/locale.gen && \
sed -i -e 's/# nb_NO.UTF-8 UTF-8/nb_NO.UTF-8 UTF-8/' /etc/locale.gen && \
Copy link
Contributor

Choose a reason for hiding this comment

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

No need in nb_NO.UTF-8 locale and tzdata reconfiguration (see http://stackoverflow.com/a/38553499/746347)

sed -i -e 's/# nb_NO.UTF-8 UTF-8/nb_NO.UTF-8 UTF-8/' /etc/locale.gen && \
echo 'LANG="nb_NO.UTF-8"'>/etc/default/locale && \
dpkg-reconfigure --frontend=noninteractive locales && \
update-locale LANG=nb_NO.UTF-8
Copy link
Contributor

Choose a reason for hiding this comment

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

en_US.UTF-8 should be here.

@juliano
Copy link
Collaborator Author

juliano commented May 22, 2017

Finally!
image

@mxl mxl merged commit 6f0c86d into zio:master May 22, 2017
@kentongray
Copy link

awesome! not to pester and I'll patiently wait but I'm eager to try this out, will this be auto published as a snapshot or will I need to wait till the next release?

@mosyp
Copy link
Collaborator

mosyp commented May 22, 2017

@kentongray each successful build in master is published as a snapshot to sonatype :)

@mxl mxl mentioned this pull request May 22, 2017
5 tasks
@kentongray
Copy link

Just letting you know I tested the ms sql support and initial tests were super promising! Love this lib! Thanks everyone

@juliano juliano deleted the sqlserver branch May 27, 2017 09:27
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.

5 participants