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

Stream the sql content #654

Closed
wants to merge 6 commits into from
Closed

Stream the sql content #654

wants to merge 6 commits into from

Conversation

npeters
Copy link

@npeters npeters commented Nov 30, 2013

  • use sql batch mode to speed the exécution
  • stream all the read operation on the sql scripts

@axelfontaine
Copy link
Contributor

Thank you very much for this! This is something I've had on the agenda for a long time (see #94) and it's great you tackled it!

I have a couple of things I am unsure about, that would need to be cleaned up before merging:

  • Make the pull request mergeable again (it currently conflicts)
  • Add a testcase that shows that the correct statements is returned in case it errors out in the middle of the batch
  • Test and ensure this works properly with SQL Server (there is already some kind of batch support built-in by using go as a delimiter)
  • Clean up the many blank lines in the test dump file.

Also the fact you seem to now generate different checksum values than before (my untested suspicion by reading the code) should really be remedied if possible. I care very strongly about compatibility and new releases should be drop-in replacements as far as possible.

Alternatively, splitting the pull request in 2 between the batch and the streaming support would also make things easier to merge.

Do you see any other things that have to be taken care of?

Keep up the great work!
Axel

@npeters
Copy link
Author

npeters commented Dec 4, 2013

I finally split the PR.
This PR is only the streaming support.
I do not think there was a probleme with the checksum: the test check that the old and the new version give the same result.
I hope it will be easier to integrate.

@axelfontaine
Copy link
Contributor

Please check the travis-ci build. It seems to be broken...

@npeters
Copy link
Author

npeters commented Dec 5, 2013

I fix it

@npeters
Copy link
Author

npeters commented Dec 5, 2013

Fix issue #615

@axelfontaine
Copy link
Contributor

Hi Nicolas,

just wanted to let you know that I haven't forgotten you. As both the streaming and the batch support are rather fundamental changes in the parser and the executor, I want to have some time to test them before going live.

Next week I'll release 2.3. My plan is to merge your work shortly after that and make it part of the Flyway 3.0 release.

Cheers
Axel

@npeters
Copy link
Author

npeters commented Dec 14, 2013

Hi Alex

I'm not worry, I follow your work in flyway and I quite impressed !

I'm waiting for the beginning of the 3.0 and I hope I will be able to help you. :)

@rajito
Copy link

rajito commented Feb 27, 2014

Hello Axel,

Any idea on when 3.0 would be available with the batch functionality?

For my project, I have made some small changes to the source to use batch updates, which significantly speeds up the migrations. Is it worth submitting a feature/pull request for it at some point in the next couple weeks?

Thank you,
Raj

@axelfontaine
Copy link
Contributor

Yes, pull requests for 3.0 are welcome. Keep the scope of each pull request small (one feature or one bugfix). I expect to release 3.0 before the end of Q1.

@rajito
Copy link

rajito commented Feb 27, 2014

Sounds good.

@axelfontaine
Copy link
Contributor

Hi Nicolas,

do you want to submit a small PR for batch support in 3.0? I plan a release next week. If not, no worries, then we'll tackle this for 3.1.

Cheers
Axel

@axelfontaine axelfontaine added this to the Flyway 3.0 milestone Apr 15, 2014
@npeters
Copy link
Author

npeters commented Apr 15, 2014

Hello Axel

I'm not plan to submit PR in 3.0 but I will do it for the 3.1

@axelfontaine
Copy link
Contributor

Sounds good. Then we'll have more time to test everything.

@axelfontaine axelfontaine modified the milestones: Flyway 3.1, Flyway 3.0 Apr 15, 2014
@rajito
Copy link

rajito commented Apr 15, 2014

Apologies for not getting back to this. I got dragged onto a different project and never got around to making the batch support code into production quality.

@CowChris
Copy link

What's the deelio here? Looks like the solution to the memory errors on large sql files got fixed, but it's held up because of a merge conflict.

So close!! :'(

Conflicts:
	flyway-core/src/main/java/org/flywaydb/core/internal/dbsupport/JdbcTemplate.java
	flyway-core/src/main/java/org/flywaydb/core/internal/dbsupport/SqlScript.java
	flyway-core/src/main/java/org/flywaydb/core/internal/resolver/sql/SqlMigrationResolver.java
	flyway-core/src/main/java/org/flywaydb/core/internal/util/scanner/Resource.java
	flyway-core/src/test/java/org/flywaydb/core/internal/dbsupport/SqlScriptSmallTest.java
@axelfontaine axelfontaine modified the milestones: Flyway 4.0, Flyway 3.2 Mar 5, 2015
@ghost
Copy link

ghost commented Aug 6, 2015

Hi,

What's the state of this? Is it going to be in Flyway 4.0? It looked like there were some problem with merging but the implementation is done. So what's the plan? Thanks!

@axelfontaine axelfontaine modified the milestones: Flyway 4.1, Flyway 4.0 Dec 30, 2015
@axelfontaine axelfontaine modified the milestones: Flyway 5.0, Flyway 4.1 Feb 6, 2017
@axelfontaine
Copy link
Contributor

Closing as this has been implemented differently as part of #615.

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.

None yet

4 participants