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

introduces migrations, add_id_property task #480

Merged
merged 14 commits into from
Sep 21, 2014
Merged

Conversation

subvertallchris
Copy link
Contributor

An easy way to make systemwide changes. Unlike ActiveRecord, it does not (yet?) keep track of migrations to run during a new deployment; rather, it offers a process for performing one-time or scheduled tasks as needed.

Usage is simple.

Include require 'neo4j/tasks/rails' in your Rakefile.

rake neo4j:migrate[task_name,subtask]

Gem-provided migrations are found in neo4j/migration.rb but if you have a custom task, put it in /db/neo4j-migrate/ and name it to match your task name. The contents of your file should be the classified version of the task name: task add_id_property would have file add_id_property.rb and contain class AddIdProperty.

In the file, your class must inherit from Neo4j::Migration and contain a migrate method, to be executed when no subtask is provided at the command line; thus, rake neo4j:migrate[add_id_property] will call Neo4j::Migration::AdIdProperty.new.migrate.

If you have other tasks that may be needed, such as setup, define those as instance methods in your class and call from command line as rake neo4j:migrate[your_task,setup].

This includes one built-in task, add_id_property. rake neo4j:migrate[add_id_property,setup] will create add_id_property.yml at db/neo4j-migrate/. Inside the provided models array, give it constants of classes that need IDs (maybe old data, maybe data added by something other than ActiveNode) and run it. rake neo4j:migrate[add_id_property]. It does it in batches of 900 nodes due to possible memory limitations; I'd love to find a way to bring this higher reliably but frankly, I don't think it really matters since it never actually returns nodes.

No tests for this, unfortunately. Just finished up and I gotta run, but I tested the built-in task thoroughly with local data.

@subvertallchris
Copy link
Contributor Author

#478

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 287772c on migration into a00d124 on master.

@subvertallchris
Copy link
Contributor Author

Documented at https://github.com/neo4jrb/neo4j/wiki/Neo4j-v3-Migrations because I'm afraid I'd forget to do it later.

@cheerfulstoic
Copy link
Contributor

Ah, very nice, thanks ;) I was playing around with assigning IDs with a query as well and was even considering using the batch REST endpoint to make a bunch of queries along the lines of MATCH (n:Label) WITH n LIMIT 1 SET n.uuid="hardcoded-value" but this is nicer. If these are strings does the join(',') work without needing to put quotes around them? Could we maybe use params?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b74e26e on migration into 5392cf0 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b74e26e on migration into 5392cf0 on master.

@cheerfulstoic
Copy link
Contributor

A couple of notes/questions:

Can we make the migrations be under db/migrations/neo4j or maybe db/neo4j/migrations?

Also, the rake task is generating a file. Maybe that should be a rails generator?

I was also a bit annoyed because I needed to add require 'neo4j/tasks/rails' to by rails app's Rakefile. Going to look into how to auto-magically include rake tasks into rails apps

@coveralls
Copy link

Coverage Status

Coverage decreased (-51.21%) when pulling 4f9fe25 on migration into 5392cf0 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-51.21%) when pulling 4c821f8 on migration into 5392cf0 on master.

@cheerfulstoic
Copy link
Contributor

I just added some code to show progress during the uuid migration including a standardized time per node as well as a MAX_PER_BATCH env variable so that people can tune to their DBs.

I'm having lots of trouble importing with my second largest label, though (1.8 million nodes). Trying to do queries to find nodes without UUIDs are just timing out... Can't seem to find any information anywhere about wether or not indexes cover null values, but it seems like they don't...

@coveralls
Copy link

Coverage Status

Coverage decreased (-51.21%) when pulling 6867a35 on migration into 5392cf0 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-51.21%) when pulling 15e8b9a on migration into 5392cf0 on master.

@cheerfulstoic
Copy link
Contributor

So nevermind what I said, I was querying without a LIMIT in the console and it was conflating my view.

Also, just pushed a bit more code to the migration so that if the migration queries are too big it will lower the batch size by 80% each time until it starts working.

@coveralls
Copy link

Coverage Status

Coverage decreased (-51.21%) when pulling 384f69a on migration into 5392cf0 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-51.21%) when pulling 3941e70 on migration into 5392cf0 on master.

@subvertallchris
Copy link
Contributor Author

That is all great! Working backwards, I wasn't sure params would help because they let the server cache query paths and these queries aren't going to happen enough that I expected that to help. Thinking about it now, though, if you have to do them in a dozen or more batches, it can't hurt.

As far as where it puts them, I wanted them separate from ActiveRecord's migrations, so db/neo4j-migrate felt like a good way to make it clear what was in there. Adding db/neo4j to .gitignore is probably a very common thing to do -- I know I did it -- so a migration folder needs to be separate from that. If you don't think there's a problem with putting the files with ActiveRecord's migrations, I'm OK with moving it into plain ole' db/migrate.

As for moving the setup into the generator, let's leave it alone for now to keep it simple. If we find that running setup through rake is annoying or confusing to people, we can move it around.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) when pulling c466aa3 on migration into 5392cf0 on master.

@subvertallchris
Copy link
Contributor Author

TAKE THAT, TRAVIS!

…was confusing, nitpicky refactor of a few old lines cause why not
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) when pulling 4da6b86 on migration into 5392cf0 on master.

@subvertallchris
Copy link
Contributor Author

OK, just made sense of this crap. The gem doesn't know about the railtie, so you need that load method in /neo4j/Rakefile to run tasks from within that directory. The lack of that was causing the Travis failure. Rails, on the other hand, doesn't care about the gem's Rakefile, so it needs the load methods in that rake_tasks block within the Railtie.

I also renamed rails.rake to migration.rake. All the different Rails, Rakefiles, railties, rake stuff was making it hard to sort out what each file was doing. That rakefile is for Rails but it handles migrations, so it's migration.rake for now.

@cheerfulstoic
Copy link
Contributor

Great, thanks for figuring it out ;) I had to run off to dinner before I could finish. Happy to see this PR merged whenever you're ready

@subvertallchris
Copy link
Contributor Author

Yeah, let's go ahead and merge now since the default id property change has been merged. I'm going to add some tests tonight or tomorrow that will just test the migrate method, we can worry about actually testing rake later. Also finishing up another task that will add or change _classname properties across a database.

subvertallchris added a commit that referenced this pull request Sep 21, 2014
introduces migrations, add_id_property task
@subvertallchris subvertallchris merged commit 36b6d32 into master Sep 21, 2014
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.

4 participants