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

Sublevel #1261

Closed
wants to merge 1 commit into from
Closed

Sublevel #1261

wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Member

in progress, @crodjer's #1245 is a godsend for this one

@calvinmetcalf
Copy link
Member Author

problem here seems to be stemming from the all dbs, if anyone else wants to take a stab at using sublevel instead of fs I wouldn't be offended

@nick-thompson
Copy link
Contributor

Yeh I was looking at this, came to the same conclusion. I'll give it another look later tonight/tomorrow.

@daleharvey
Copy link
Member

If someone did https://github.com/daleharvey/pouchdb/issues/1351 then those issues could go away :)

@calvinmetcalf
Copy link
Member Author

Which reminds me, #1351 was pretty what I was having in mind for
simplifying it
On Feb 11, 2014 6:27 PM, "Dale Harvey" notifications@github.com wrote:

If someone did #1351 https://github.com/daleharvey/pouchdb/issues/1351then those issues could go away :)

Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1261#issuecomment-34821574
.

@calvinmetcalf
Copy link
Member Author

ready for review (well after I squash it) this also fixes a few issues with ids, like how they weren't really promises, and for some reason indexdb and websql where using uuids but leveldb wasn't.

this is backwards compatible and will migrate newer databases automatically.

@calvinmetcalf
Copy link
Member Author

AND IT'S GREEN!

@daleharvey
Copy link
Member

This looks awesome, good job

I think its worth thinking a little bit about the migration script, its currently setup as 'oldstyle to 'new', which isnt really maintainable, if we need a new migration, I was about to suggest tying it to releases but that is a bad idea too, I we should as couch / indexeddb do and have a internal db_version number where we do migrations between specific db_versions, this means we can drop support for old ones etc

It would be really nice to take a look at https://github.com/daleharvey/pouchdb/pull/1340 and see how much parity we can get between what the migrations look like, I havent done that yet, gonna look at it properly in the morning

The test run for this looks frickin amazing - https://travis-ci.org/daleharvey/pouchdb/builds/19376030

@calvinmetcalf
Copy link
Member Author

so the term migration might be a misnomer as I'm not actually migrating any
of the internal things, but instead migrating the external setup, aka all 4
stores continue to be identical but instead of 4 dbs in a folder they are 4
sublevels inside a level. In other words you cant't store the version
number because we are changing where the data storage is.

On Fri, Feb 21, 2014 at 9:21 PM, Dale Harvey notifications@github.comwrote:

This looks awesome, good job

I think its worth thinking a little bit about the migration script, its
currently setup as 'oldstyle to 'new', which isnt really maintainable, if
we need a new migration, I was about to suggest tying it to releases but
that is a bad idea too, I we should as couch / indexeddb do and have a
internal db_version number where we do migrations between specific
db_versions, this means we can drop support for old ones etc

It would be really nice to take a look at #1340https://github.com/daleharvey/pouchdb/pull/1340and see how much parity we can get between what the migrations look like

The test run for this looks frickin amazing -
https://travis-ci.org/daleharvey/pouchdb/builds/19376030

Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1261#issuecomment-35792063
.

-Calvin W. Metcalf

});
from.pipe(to);
}
fs.unlink(base + '.uuid', function (err) {
Copy link
Member Author

Choose a reason for hiding this comment

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

try to delete the .uuid file, if it fails then we know it isn't an old style pouch

@calvinmetcalf
Copy link
Member Author

I though of 2 things that this needs so don't merge quite yet

@calvinmetcalf
Copy link
Member Author

ok good if green

@daleharvey
Copy link
Member

Its green, node test is at least

I wont get to it for about an hour, but I want a chance to try a small change that makes the migration clearer, its not changing what happens, but it will make me feel better when we come to delete some of this stuff

But aside from that, +1000

@calvinmetcalf
Copy link
Member Author

so the 2 changes i made were

  • not migrating if the user passed the db option (for choosing non-leveldown dbs, this is a leveldb option) or passes a noMigrate options.
  • waiting until all of the old dbs are loaded into the new db before deleting them, this should prevent dbs getting stuck in a half migrated state. we still delete the .uuid file first but I can't think of any other way preventing a race condition, this means that the worst possible situation can be fixed with the command touch pouchname.uuid

@calvinmetcalf
Copy link
Member Author

a way to phase this out is for noMigrate to default to true in v1.3 or 1.4 and then in version 2 remove it

//id.should.not.be.empty();
if (id[id.length - 1] === '/') {
id = id.slice(0, -1);
} else if (id.slice(0, 7) === '_pouch_') {
Copy link
Member

Choose a reason for hiding this comment

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

I dont think we should ever return prefixed dbnames via the public api

@daleharvey
Copy link
Member

So I wrote the migration script, and you were right, due to having to do the version tests before we open the database, and to have to run the migration script before we complete opening the database this cant be done inside the same mechanism in which we do schema upgrades, it doesnt end up making anything easier to understand

There were some style changes that are against our style guide, but in general this file has always broken them so not a big deal, we use UCASE for constants, so STORES never should have been ucase, but DOC_STORE should be.

Due to this being a large patch with a bunch of things depending on it, nolans stuff and I have patches for level in progress, I will merge now and file an issue to address the .id issue

For anyone following along, this patch speeds up the leveldb adapter by 2x, and fixes a huge amount of intermittent bugs in the node tests

@calvinmetcalf
Copy link
Member Author

Can you open an issue for the UPPER_CASE one as well.
On Feb 22, 2014 7:13 PM, "Dale Harvey" notifications@github.com wrote:

So I wrote the migration script, and you were right, due to having to do
the version tests before we open the database, and to have to run the
migration script before we complete opening the database this cant be done
inside the same mechanism in which we do schema upgrades, it doesnt end up
making anything easier to understand

There were some style changes that are against our style guide, but in
general this file has always broken them so not a big deal, we use UCASE
for constants, so STORES never should have been ucase, but DOC_STORE should
be.

Due to this being a large patch with a bunch of things depending on it,
nolans stuff and I have patches for level in progress, I will merge now and
file an issue to address the .id issue

For anyone following along, this patch speeds up the leveldb adapter by
2x, and fixes a huge amount of intermittent bugs in the node tests

Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1261#issuecomment-35819616
.

@calvinmetcalf
Copy link
Member Author

Probably a good idea not to return to return the prefix, I assumed that was
intentional
On Feb 22, 2014 7:24 PM, "Calvin Metcalf" calvin.metcalf@gmail.com wrote:

Can you open an issue for the UPPER_CASE one as well.
On Feb 22, 2014 7:13 PM, "Dale Harvey" notifications@github.com wrote:

So I wrote the migration script, and you were right, due to having to do
the version tests before we open the database, and to have to run the
migration script before we complete opening the database this cant be done
inside the same mechanism in which we do schema upgrades, it doesnt end up
making anything easier to understand

There were some style changes that are against our style guide, but in
general this file has always broken them so not a big deal, we use UCASE
for constants, so STORES never should have been ucase, but DOC_STORE should
be.

Due to this being a large patch with a bunch of things depending on it,
nolans stuff and I have patches for level in progress, I will merge now and
file an issue to address the .id issue

For anyone following along, this patch speeds up the leveldb adapter by
2x, and fixes a huge amount of intermittent bugs in the node tests

Reply to this email directly or view it on GitHubhttps://github.com/daleharvey/pouchdb/pull/1261#issuecomment-35819616
.

@daleharvey
Copy link
Member

merged: daleharvey@b53ceda

I dont think we need to do an opts.noMigrate deprecation, its not an option that users are gonna see or care about, it may be useful for people to exclude a bunch of code when they are testing memdown / idb backends

Also I think we should agree on how long to support this, I specifically call out the node implementation to not support migrations in the docs, http://pouchdb.com/learn.html, I think given that its called out as being unsupported, and anyone that is using pouch needs to be keeping up to date or a ton is changing between the, I say we make sure to keep it in the next 2 releases then its free to get deleted

For schema migrations we will do longer support cycles, but this is one off migration (on the server, so users have control over it) so I would like to not have that code around

@daleharvey daleharvey closed this Feb 23, 2014
@daleharvey
Copy link
Member

I dont think its worth tracking code nit issues, can just fix them as we see them, just something to remember in reviews, I fixed the ones I touched in the uuid fix

@daleharvey daleharvey deleted the sublevel branch February 23, 2014 22:46
@nolanlawson nolanlawson mentioned this pull request Feb 24, 2014
sygi pushed a commit to sygi/pouchdb that referenced this pull request Apr 28, 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.

None yet

3 participants