Make debugging index creation dramatically easier #440

williscool · 2018-10-15T02:16:59Z

Currently Its not obvious how index's are managed in dynamoose.

Creation using

dynamoose.model(name, schema, {update: true})

as mentioned in #221

Will delete the indexes that are currently on a table. Which is a HUGE NO NO in production.

Especially since they can take non trivial amounts of time to get back setup... which at best causes service degradation and at worse downtime.

This new change allows you to use

DEBUG=dynamoose:table*  <how ever you start your server i.e. node server.js or yarn serve>

And see the difference in the console i.e.

  dynamoose:table compareIndexes +0ms
indexs for compare local: { IndexName: 'fb_id-index',
  KeySchema:
   [ { AttributeName: 'fb_id', KeyType: 'HASH' },
     { AttributeName: 'fb-id', KeyType: 'RANGE' } ],
  Projection: { ProjectionType: 'ALL' } } remote: { IndexName: 'fb_id-index',
  KeySchema: [ { AttributeName: 'fb_id', KeyType: 'HASH' } ],
  Projection: { ProjectionType: 'ALL' } }
indexs for compare local: { IndexName: 'phone_number-index',
  KeySchema:
   [ { AttributeName: 'phone_number', KeyType: 'HASH' },
     { AttributeName: 'phone_number', KeyType: 'RANGE' } ],
  Projection: { ProjectionType: 'ALL' } } remote: { IndexName: 'phone_number-index',
  KeySchema: [ { AttributeName: 'phone_number', KeyType: 'HASH' } ],
  Projection: { ProjectionType: 'ALL' } }


blah blah blah

  dynamoose:table devTickets indexes are not synchronized and update flag is set to false +1ms

You can use this information to figure out what is different between the indexes on the table that exist versus the ones that you have setup in your dynamoose model.

And change accordingly

MUUCH better way to deal with

Error: indexes are not synchronized and update flag is set to false

Than before

Type (select 1):

Bug fix
Feature implementation
Documentation improvement
Testing improvement

Test added to report bug (GitHub issue #--- @---)
Something not listed here

Is this a breaking change? (select 1):

🚨 YES 🚨
No
I'm not sure

Is this ready to be merged into Dynamoose? (select 1):

Yes
No

Are all the tests currently passing on this PR? (select 1):

Yes
No

Other:

I have searched through the GitHub pull requests to ensure this PR has not already been submitted
I have updated the Dynamoose documentation (if required) given the changes I made
I have added/updated the Dynamoose test cases (if required) given the changes I made
I have run npm test from the root of the project directory to ensure all tests continue to pass
I agree that all changes made in this pull request may be distributed and are made available in accordance with the Dynamoose license
All of my commits and commit messages are detailed, explain what changes were made, and are easy to follow and understand
I have confirmed that all my code changes are indented properly using 2 spaces
I have filled out all fields above

coveralls · 2018-10-15T02:19:28Z

Pull Request Test Coverage Report for Build 686

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 84.953%

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	2	76.76%

Totals
Change from base Build 682:	-0.1%
Covered Lines:	1876
Relevant Lines:	2135

💛 - Coveralls

williscool · 2018-10-15T09:29:37Z

Also happy to update any docs if you point me their way

fishcharlie

One question

fishcharlie · 2018-10-15T16:39:02Z

lib/Table.js

@@ -40,6 +40,7 @@ const compareIndexes = function compareIndexes(local, remote) {
        let localIndex = (({ IndexName, KeySchema, Projection }) => ({ IndexName, KeySchema, Projection }))(localIndexes[i]);
        let remoteIndex = (({ IndexName, KeySchema, Projection }) => ({ IndexName, KeySchema, Projection }))(remoteIndexes[j]);

+        debug("\nindex being compared \nlocal:", localIndex, "\nremote: \n", remoteIndex)


What are the thoughts about changing this to one of the following options? Not sure if this is a good idea, and I'm not sure what we are doing in cases like this in other areas.

Not sure if I'm the biggest fan of those \n in the string. Especially since I'm not sure how it actually looks in the console because debug appends that string and other information to the beginning of every debug statement. So I'm not sure if it'd look really weird with those line breaks.

debug("index being compared, local:", localIndex, ", remote: ", remoteIndex)

debug("index being compared"); debug("local: ", localIndex); debug("remote: ", remoteIndex);

Not sold on my ideas, or that you are doing it wrong. Maybe your way is the best. Just wondering what your thoughts are in terms of this.

If you could also look throughout the project and see if you can find other areas where we are doing \n or one of my methods, that'd be awesome too.

I actually prefer it your way. was just hacking it out to see if it worked when I wrote that lol. fixing it update

fishcharlie · 2018-10-15T16:41:24Z

Thanks for this @williscool!! There is no documentation updated needed for this PR.

I do need you to check all of the boxes under the Other: section of the PR template tho after reading/agreeing to/understanding each one. That needs to be done before merging.

Other than that and addressing my question in the review I think we should be good to merge this in pretty soon.

Thanks again!

williscool · 2018-10-16T05:14:36Z

Will get you this stuff ASAP.

Thanks for looking at this!

Currently Its not obvious how index's are managed created in dynamoose using ``` dynamoose.model(name, schema, {update: true}) ``` Will *delete the indexes that are currently on a table*. Which is a HUGE NO NO in production. Especially since they can take non trivial amounts of time to get back setup... which at best causes service degradation and at worse downtime This new change allows you to use ``` DEBUG=dynamoose:table* <how ever you start your server i.e. node server.js or yarn serve> ``` And see the difference in the console i.e. ``` dynamoose:table compareIndexes +0ms indexs for compare local: { IndexName: 'fb_id-index', KeySchema: [ { AttributeName: 'fb_id', KeyType: 'HASH' }, { AttributeName: 'fb-id', KeyType: 'RANGE' } ], Projection: { ProjectionType: 'ALL' } } remote: { IndexName: 'fb_id-index', KeySchema: [ { AttributeName: 'fb_id', KeyType: 'HASH' } ], Projection: { ProjectionType: 'ALL' } } indexs for compare local: { IndexName: 'phone_number-index', KeySchema: [ { AttributeName: 'phone_number', KeyType: 'HASH' }, { AttributeName: 'phone_number', KeyType: 'RANGE' } ], Projection: { ProjectionType: 'ALL' } } remote: { IndexName: 'phone_number-index', KeySchema: [ { AttributeName: 'phone_number', KeyType: 'HASH' } ], Projection: { ProjectionType: 'ALL' } } blah blah blah dynamoose:table devTickets indexes are not synchronized and update flag is set to false +1ms ``` MUUCH better way to deal with ``` Error: indexes are not synchronized and update flag is set to false ```

williscool · 2018-10-18T00:46:44Z

@fishcharlie updated things!

fishcharlie

LGTM

fishcharlie · 2018-10-18T04:41:15Z

@williscool This looks good! I will get this merged in soon, and hopefully we will have a release coming up soon so that we can get it on NPM. We do have some bug fixes I'm hoping get fixed up soon so we can do a release.

Thanks for your contribution!!

williscool · 2018-10-18T07:26:55Z

❤️

fishcharlie requested changes Oct 15, 2018

View reviewed changes

fishcharlie added the status:do-not-merge label Oct 15, 2018

williscool force-pushed the patch-1 branch from 5903078 to 85c8e89 Compare October 18, 2018 00:45

fishcharlie approved these changes Oct 18, 2018

View reviewed changes

fishcharlie removed the status:do-not-merge label Oct 18, 2018

fishcharlie mentioned this pull request Oct 18, 2018

Version 1.1.0 #450

Merged

fishcharlie merged commit 679fa86 into dynamoose:master Oct 18, 2018

dolsem mentioned this pull request Oct 26, 2018

Index gets re-created on every restart without good reason #453

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make debugging index creation dramatically easier #440

Make debugging index creation dramatically easier #440

williscool commented Oct 15, 2018 •

edited

Loading

coveralls commented Oct 15, 2018 •

edited

Loading

williscool commented Oct 15, 2018

fishcharlie left a comment

fishcharlie Oct 15, 2018

williscool Oct 18, 2018

fishcharlie commented Oct 15, 2018

williscool commented Oct 16, 2018

williscool commented Oct 18, 2018

fishcharlie left a comment

fishcharlie commented Oct 18, 2018

williscool commented Oct 18, 2018

Make debugging index creation dramatically easier #440

Make debugging index creation dramatically easier #440

Conversation

williscool commented Oct 15, 2018 • edited Loading

Type (select 1):

Is this a breaking change? (select 1):

Is this ready to be merged into Dynamoose? (select 1):

Are all the tests currently passing on this PR? (select 1):

Other:

coveralls commented Oct 15, 2018 • edited Loading

Pull Request Test Coverage Report for Build 686

💛 - Coveralls

williscool commented Oct 15, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

fishcharlie Oct 15, 2018

Choose a reason for hiding this comment

williscool Oct 18, 2018

Choose a reason for hiding this comment

fishcharlie commented Oct 15, 2018

williscool commented Oct 16, 2018

williscool commented Oct 18, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

fishcharlie commented Oct 18, 2018

williscool commented Oct 18, 2018

williscool commented Oct 15, 2018 •

edited

Loading

coveralls commented Oct 15, 2018 •

edited

Loading