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

bootstrap directory? #93

Closed
chb0github opened this issue Oct 17, 2017 · 14 comments
Closed

bootstrap directory? #93

chb0github opened this issue Oct 17, 2017 · 14 comments

Comments

@chb0github
Copy link
Contributor

The bootstrap file is a good embrace of real life but could be improved by alternatively supporting a directory called boostrap My situation, for example, would have a single sql file that's 500mb. This could actually prevent a migration from running because it just requires too much ram. So, breaking files down makes good sense.

I can make the PR no problem.

@harawata
Copy link
Member

Hi @chb0github ,

Thank you for the report.
I could verify the problem with a large bootstrap.sql.
Regarding how to solve it, though, I think it's better to improve Migrations so that it can handle a large file efficiently.
I will look into it.

In case you (or someone) need a quick solution (and have a lot of RAM), try increasing the heap size.
The heap size is specified at the bottom of the launcher script ($MIGRATIONS_HOME/bin/migrate).
The default seems to be 500MB (-Xmx500m).

@chb0github
Copy link
Contributor Author

I think you misunderstand: I don't currently have this issue. I am suggesting an alternative approach. A directory called bootstrap that is filled with SQL files. It makes management easier and can reduce the memory foot print

@harawata
Copy link
Member

Bootstrap is a one-time operation and bootstrap SQLs are not supposed to be modified once migration has started.
So, I thought you could just concatenate those scripts with cat or type before performing bootstrap once the memory usage issue was fixed.

If this turns out to be a frequently requested feature, I would add some general purpose way to achieve it (e.g. add bootstrap hooks or allow creating a custom FileMigrationLoader) rather than implementing it as a built-in feature.

@chb0github
Copy link
Contributor Author

The use of conventions in this project is a good thing - it eliminates confusion. You could support a custom migration loader if you have a truly crazy scenario.

My case: I have a bootstrap directory with 80+ sql files each one representing the creation of a schema in mysql along with several for domain data. While I can concatenate them that makes a single file unwieldy. The implementation could be really easy:

File bootstrap = new File("./bootstrap");
if(bootstrap.exists() && bootstrap.isDirectory())
   File[] files = bootStrap.listFiles(f -> f.getName().endsWith(".sql"));

I guess what you're saying is: if we can't make it 100% flexible then don't make it flexible at all.

@harawata
Copy link
Member

Just FYI, the latest snapshot should be able to read a large script file without throwing OutOfMemoryError.

Regarding the proposed change, I still prefer providing flexibility in one of the two ways I proposed rather than adding a variation to the core schema.

While I can concatenate them that makes a single file unwieldy

You can delete the concatenated file after performing bootstrap and keep maintaining the 80+ files.
So, with bootstrap hooks, my idea was to concatenate the original files in pre-bootstrap hook and to delete the concatenated file in post-bootstrap hook.
It's not pretty, but good enough for a one-time operation, maybe? :)

@chb0github
Copy link
Contributor Author

Our situation is that we are actually using this as part of a build process and creating docker images of our DB - so, it's repeated over-and-over. Though the bootstraps files should never be changed, it doesn't mean people won't want to look at them to find out quickly where something is.

@chb0github
Copy link
Contributor Author

I can rollout this feature in a completely backward compatible way:

if(bootStrap.isfile())
   loadBootStrap(bootStrap);
else
   bootStrap.listFiles().stream().forEach(this::loadBootstrap)

I am also willing donate a dockerfile as per #105 to see this change accepted

@harawata
Copy link
Member

I am working on the custom-FileMigrationLoader implementation.
It might take some more time as I'll be busy with my day job for a while.

@chb0github
Copy link
Contributor Author

chb0github commented Dec 12, 2017 via email

@harawata
Copy link
Member

Thanks, @chb0github !
The implementation is done actually. It still needs test cases and documentation.
It may be possible to commit the code first for testing, though.
I'll open a PR later and ask @h3adache his opinion about the implementation.

@chb0github
Copy link
Contributor Author

@harawata - what's the situation on your implementation?

@harawata
Copy link
Member

Hi @chb0github ,

You mean the tests and doc, right? I'm still working on it.

Have you had the chance to try the custom FileMigrationLoader that I attached to #107 ?
Did it work as you expected?

I'll close this ticket as the requirement can be met with #107 .

@chb0github
Copy link
Contributor Author

chb0github commented Dec 29, 2017 via email

@harawata
Copy link
Member

Yes! Actually I implemented one already.

That's awesome. Thank you for the clarification!
When it comes to database schema migration, every team has some unique requirements, it seems. 😁

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

No branches or pull requests

2 participants