noppoMan / npdynamodb

A Node.js Simple Query Builder and ORM for AWS DynamoDB
112 stars 19 forks source link

waitUntilTableActivate and all indexes are active #57

Closed sean-krail closed 6 years ago

sean-krail commented 6 years ago

Right now waitUntilTableActivate('tableName') does not wait for the global secondary indexes to be active before resolving. This function (or maybe another function) should not resolve until both the table is active and all global secondary indexes are active. This is important functionality for globalSecondaryIndex calls.

noppoMan commented 6 years ago

@sean-krail Hi, Sean Sorry for late reply. and I noticed the email from you today... Actually, npdynamodb is not dead. It's still running on our services without big problems.

Anyways, This PR looks good. I'll merge this after passing unit tests on CircleCI.

Thanks! Yuki

noppoMan commented 6 years ago

I was writing waitUntilTableActivate in every migration files that call secondary indexes updating to avoid this issue... But it's agony for common developers.

sean-krail commented 6 years ago

@noppoMan I'd like to add another migration to the unit test in order to test adding two global secondary indexes to the same table back-to-back (two migrations back-to-back). So I'd just copy /test/migrations/20150819155840_alter_test_table1.js into another file like /test/migrations/20150819155841_alter_test_table1_2.js and change references of indexName3 to indexName4 and hash_key2 to hash_key3.

This has been the core of our issue and why I opened this PR. If the unit test passes, everything will be good to go for us.

sean-krail commented 6 years ago

@noppoMan I've added a new migration to test creating a global secondary index directly after creating one in the same table in the previous migration. This will test if the updated waitUntilTableActivate function blocks properly and doesn't have any race condition issues.

noppoMan commented 6 years ago

@sean-krail

thanks! to add a practical test. I'm just trying to migrate CI from CircleCI to Travis to test on various Node.js major versions. https://github.com/noppoMan/npdynamodb/pull/58

If it goes green, I'll merge it to master first. Does it work for you?

noppoMan commented 6 years ago

It seems perfect. https://travis-ci.org/noppoMan/npdynamodb

noppoMan commented 6 years ago

@sean-krail I merged #58 to master. So could you rebase origin/master and push again? Then, test will start on travis.

sean-krail commented 6 years ago

@noppoMan see the error in Travis? This is the exact problem we are having. For some reason it's not really blocking until the table is fully activated. It may be timing out, but I'm not sure.

noppoMan commented 6 years ago

@sean-krail I checked the error in Travis. and I might solved this problem with following commit https://github.com/noppoMan/npdynamodb/commit/57b99a892092229bf30d18b4d6e7a4aa7a365fd2 on waitUntilTableActivate-patch branch.

We also need not only your changes but also waiting for the table(that is used for migration states management) activation on run/rollback as like following to avoid Unhandled rejection LimitExceededException: Subscriber limit exceeded: Only 1 online index can be created or deleted simultaneously per table error. https://github.com/noppoMan/npdynamodb/commit/57b99a892092229bf30d18b4d6e7a4aa7a365fd2#diff-d1c77b25edfb118196b1771e0c949374R158 https://github.com/noppoMan/npdynamodb/commit/57b99a892092229bf30d18b4d6e7a4aa7a365fd2#diff-d1c77b25edfb118196b1771e0c949374R197

Please make sure this work well with you or not. If It's ok, Could you add the diffs(between this PR and waitUntilTableActivate-patch branch) to this PR? or If you give me a right privilege for your folked repository, I can commit and push the diffs.

noppoMan commented 6 years ago

The test is passed. https://travis-ci.org/noppoMan/npdynamodb/builds/294381798

sean-krail commented 6 years ago

@noppoMan Sweet! I just gave you write access to jitterbit/npdynamodb

noppoMan commented 6 years ago

@sean-krail merged with my commit. I'll merge this PR to master tomorrow(JST).

noppoMan commented 6 years ago

@sean-krail I just published version 0.2.15 that includes this PR! You can now available it with npm install

sean-krail commented 6 years ago

It's working great! Thanks again @noppoMan