-
Notifications
You must be signed in to change notification settings - Fork 35
DOCSP-34591 Add errorDescription and success keys to progress response #295
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
kanchana-mongodb
commented
Apr 3, 2024
•
edited
Loading
edited
- JIRA
- Stage
- Build
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 tweak to make, plus two other changes on the page (https://preview-mongodbkanchanamongodb.gatsbyjs.io/cluster-sync/DOCSP-34591/reference/api/progress/#response-1) that aren't in this .rst file:
First, just above the error response table:
If mongosync encounters an error, the progress endpoint returns the following field:
This should be "fields" (plural).
Second: the success
boolean field is also returned for non-error responses. I don't know if we want to document this in the table, but I think we should at least update the sample successful response to this (note the top-level "success" key):
{
"success": true,
"progress":
{
"state":"RUNNING",
"canCommit":true,
"canWrite":false,
"info":"change event application",
"lagTimeSeconds":0,
"collectionCopy":
{
"estimatedTotalBytes":694,
"estimatedCopiedBytes":694
},
"directionMapping":
{
"Source":"cluster0: localhost:27017",
"Destination":"cluster1: localhost:27018"
}
}
}
Thanks!
@@ -8,8 +8,16 @@ | |||
- Type | |||
- Description | |||
|
|||
* - ``success`` | |||
- string |
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.
This is a boolean field, not a string.
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 more tweak; thanks!
* - ``success`` | ||
- boolean | ||
- Status of the ``progress`` command. Value is ``true`` if the | ||
command succeeds and ``false`` if the command fails. |
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.
This table item is a little misleading. This table is introduced by the sentence
If mongosync successfully gets the status of the sync process, all response fields are wrapped in a top-level
progress
object with the following fields:
The success
key is a top-level field, not part of the progress
object. That is: a successful response looks like this:
{
success: true,
progress: {
// All the other fields as in the table
}
}
So, this description is fine, but it's a top-level field and not part of the progress
object.
8504dbd
to
ae91169
Compare
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.
Thanks @kanchana-mongodb! LGTM
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 % 1 small suggestion; that isn't really a technical point but a clarity one, so happy to approve without needing another review. Thanks!
source/reference/api/progress.txt
Outdated
The ``success`` field contains the status of the ``progress`` command. | ||
Value is ``true`` if the command succeeds and ``false`` if the command | ||
fails. | ||
|
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.
(nit) Since this is in the "successful response" section, we could maybe simplify by saying here that success
will always be true here.