-
Notifications
You must be signed in to change notification settings - Fork 39
DOCSP 29155 adding sort and uuids to mongosync page #230
Conversation
MongoCaleb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions.
source/reference/mongosync.txt
Outdated
| Sorting | ||
| ~~~~~~~ | ||
|
|
||
| ``mongosync`` creates a new natural (without any sort method specified) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it reads better to say "creates a new natural order (without any sort method specified) on the ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
source/reference/mongosync.txt
Outdated
| ``mongosync`` creates a new natural (without any sort method specified) | ||
| order on the destination cluster. If applications depend on document | ||
| order without a defined sort method, they may require additional updating to | ||
| work with a migrated cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be cleaned up a bit and also be more specific about what needs to be updated. Perhaps:
If applications depend on document order, but don't have a defined sort method, they may need to be updated to specify the expected sort order before they will work properly with the migrated cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
source/reference/mongosync.txt
Outdated
| UUIDs | ||
| ~~~~~ | ||
|
|
||
| ``mongosync`` creates new universally unique identifiers (UUIDs) for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we should use this:
:abbr:`UUIDs (universally unique identifiers)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! (I was not aware of this markup so thank you so much for pointing it out!)
source/reference/mongosync.txt
Outdated
| ``mongosync`` creates new universally unique identifiers (UUIDs) for | ||
| collections on the destination cluster. There is no relationship between | ||
| UUIDs on the source cluster and the destination cluster. If applications | ||
| contain hard-coded collection UUIDs (which MongoDB does not recommend), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they "collection UUIDs" or document UUIDs within a collection? If the latter, I suggest just dropping the word "collection".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
Thanks for your feedback @MongoCaleb ! Could you take another look when you get a chance? |
MongoCaleb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm LGTMing, but there's one important question to check on.
source/reference/mongosync.txt
Outdated
| between UUIDs on the source cluster and the destination cluster. If | ||
| applications contain hard-coded UUIDs (which MongoDB does not | ||
| recommend), you may need to update the application's UUIDs before the | ||
| application works properly with a migrated cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last comment. Can you actually change the UUIDs? I would be surprised if this were the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked in with Alex Komyagin who said the "The developer would need to get the new UUID's from the new cluster or update the app not to rely on UUIDs" so I changed the phrasing accordingly
FGasper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gist of what’s here is accurate. I’ve left a couple technical-nit comments, and one grammatical nit. See what you think.
Thank you!
source/reference/mongosync.txt
Outdated
| ``mongosync`` creates new :abbr:`UUIDs (universally unique identifiers)` | ||
| for collections on the destination cluster. There is no relationship |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mongosync doesn’t “create UUIDs” per se; rather, it creates collections, which have UUIDs.
| ``mongosync`` creates new :abbr:`UUIDs (universally unique identifiers)` | |
| for collections on the destination cluster. There is no relationship | |
| The collections that ``mongosync`` creates on the destination cluster | |
| have new :abbr:`UUIDs (universally unique identifiers)`. There is no relationship |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
source/reference/mongosync.txt
Outdated
| update the application to specify the expected sort order before the | ||
| application works properly with the migrated cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a grammatical nit:
| update the application to specify the expected sort order before the | |
| application works properly with the migrated cluster. | |
| update those applications to specify the expected sort order before the | |
| applications work properly with the migrated cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
source/reference/mongosync.txt
Outdated
| ``mongosync`` creates a new natural order (without any sort method | ||
| specified) on the destination cluster. If applications depend on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mongosync doesn’t “create an order”; rather, the order in which it inserts documents is undefined.
| ``mongosync`` creates a new natural order (without any sort method | |
| specified) on the destination cluster. If applications depend on | |
| ``mongosync`` inserts documents on the destination cluster in an undefined order. | |
| Thus, natural sort order is not preserved from the source to the destination. If applications | |
| depend on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
Thanks for your feedback @FGasper ! Could you take another look at this when you get a chance? |
FGasper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
* DOCSP-29155 adding behavior info to mongosync * DOCSP-29155 adding sorting and uuids to mongosync page * DOCSP-29155 adding sorting and uuids to mongosync page * DOCSP-29155 copy edits * DOCSP-29155 copy edit * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edit
* DOCSP-29155 adding behavior info to mongosync * DOCSP-29155 adding sorting and uuids to mongosync page * DOCSP-29155 adding sorting and uuids to mongosync page * DOCSP-29155 copy edits * DOCSP-29155 copy edit * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edit
* DOCSP-29155 adding behavior info to mongosync * DOCSP-29155 adding sorting and uuids to mongosync page * DOCSP-29155 adding sorting and uuids to mongosync page * DOCSP-29155 copy edits * DOCSP-29155 copy edit * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edits * DOCSP-29155 tech edit
DESCRIPTION
adding sorting and uuids information to mongosync page
STAGING
https://preview-mongodbltranmdb2.gatsbyjs.io/cluster-sync/DOCSP-29155/reference/mongosync/#uuids
JIRA
https://jira.mongodb.org/browse/DOCSP-29155
BUILD LOG
https://workerpool-boxgs.mongodbstitch.com/pages/job.html?collName=queue&jobId=6580ab867a5cb5fef55f4e19
Self-Review Checklist
External Review Requirements
What's expected of an external reviewer?