-
Notifications
You must be signed in to change notification settings - Fork 20
DOCSP-37514 - UUIDs #75
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
Conversation
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.
Really nice job on a weird page type like this. Left a few suggestions and questions that I think could clean things up a bit. Let me know if you want to discuss formatting or anything like that
source/data-formats/uuid.txt
Outdated
Originally, MongoDB represented UUIDs as BSON ``Binary`` | ||
values of subtype 3. Because subtype 3 didn't standardize the byte order of UUIDs | ||
during encoding, different MongoDB drivers encoded UUIDs with different byte orders. | ||
Use the tabs below to compare the ways in which different MongoDB language drivers |
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.
https://www.mongodb.com/docs/meta/style-guide/terminology/alphabetical-terms/#std-term-below
Use the tabs below to compare the ways in which different MongoDB language drivers | |
Use the following tabs to compare the ways in which different MongoDB language drivers |
source/data-formats/uuid.txt
Outdated
|
||
To standardize UUID byte order, we created ``Binary`` subtype 4. Although this subtype | ||
is handled consistently across MongoDB drivers, some MongoDB deployments still contain | ||
UUID values of subtype 3. These legacy UUIDs can result in highly unintuitive behavior, |
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.
UUID values of subtype 3. These legacy UUIDs can result in highly unintuitive behavior, | |
UUID values of subtype 3. These legacy UUIDs can result in unintuitive behavior, |
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.
good catch
source/data-formats/uuid.txt
Outdated
UUID values of subtype 3. These legacy UUIDs can result in highly unintuitive behavior, | ||
as shown in the following example. | ||
|
||
Example: Applications Share a MongoDB Deployment |
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 wonder if this example is particularly necessary, or if we can just summarize it in a sentence or two at the end of the last paragraph. It feels like a lot to try and follow the different applications and UUID values shown here when IMO the gist of the message is something like "Because different drivers encode UUIDs into different textual representations, finding those values with multiple applications can be unreliable."
Removing this example would also make the page seem less daunting and will get to the main points a bit quicker.
Definitely open to discussing this more if needed
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 in favor of almost anything that makes this page shorter
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.
For some context, there were originally two of these example sections (one's now obsolete), which made more sense.
source/data-formats/uuid.txt
Outdated
A Short History of MongoDB UUIDs | ||
-------------------------------- |
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 should be a heading level under Overview since it's mostly giving context to what this page is for
A Short History of MongoDB UUIDs | |
-------------------------------- | |
A Short History of MongoDB UUIDs | |
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
source/data-formats/uuid.txt
Outdated
:header-rows: 1 | ||
|
||
* - UUID Representation | ||
- Default? |
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 column can be removed. Instead the "unspecified" option could be labeled as unspecified (default)
or something similar to that
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.
Also removed heading colons, since it turns out those are against the SG
source/data-formats/uuid.txt
Outdated
UUID you're reading from MongoDB was inserted using the ``PYTHON_LEGACY`` representation. | ||
This will be true if the following criteria are met: | ||
|
||
- The UUID was inserted by an application using {+driver-short+}. |
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.
Should this be "using PyMongo versions earlier than v4.0"?
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 so
source/data-formats/uuid.txt
Outdated
|
||
Use the ``PYTHON_LEGACY`` UUID representation if the | ||
UUID you're reading from MongoDB was inserted using the ``PYTHON_LEGACY`` representation. | ||
This will be true if the following criteria are met: |
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 originally interpreted this as only one of the following needing to be met (which I think is incorrect?) Applies to the next sections as well
This will be true if the following criteria are met: | |
This will be true if both of the following criteria are met: |
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 w/ one fix
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>
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!
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>
Co-authored-by: Jordan Smith <45415425+jordan-smith721@users.noreply.github.com>
Pull Request Info
PR Reviewing Guidelines
JIRA - https://jira.mongodb.org/browse/DOCSP-37514
Staging - https://preview-mongodbmongokart.gatsbyjs.io/pymongo/docsp-37514-uuid/data-formats/uuid/
Self-Review Checklist