Skip to content
This repository was archived by the owner on May 1, 2025. It is now read-only.

Conversation

@ltran-mdb2
Copy link
Collaborator

@ltran-mdb2 ltran-mdb2 commented Dec 13, 2023

DESCRIPTION

adding information to the commit page to clarify behavior

STAGING

https://preview-mongodbltranmdb2.gatsbyjs.io/cluster-sync/DOCSP-28174/reference/api/commit/

JIRA

https://jira.mongodb.org/browse/DOCSP-28174

BUILD LOG

https://workerpool-boxgs.mongodbstitch.com/pages/job.html?collName=queue&jobId=6579e76f7a5cb5fef5c7c6c4

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Is this free of spelling errors?
  • Is this free of grammatical errors?
  • Is this free of staging / rendering issues?
  • Are all the links working?

External Review Requirements

What's expected of an external reviewer?

Copy link
Collaborator

@ajhuh-mdb ajhuh-mdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one suggestion!

Description
-----------

Commits the synchronization operation to the destination cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] It looks like the first half of the new sentence is saying virtually the same thing as the first sentence that's already here. Maybe we can just merge these two?

Suggested change
Commits the synchronization operation to the destination cluster.
Finalizes the synchronization operation between the source and destination clusters. ``commit``stops continuous...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only hesitation here is that the the other endpoints use the endpoint name to describe the definition of the endpoint behavior. I do like the idea of combining the sentences and using "finalize" rather than "commit" but just wanted to check in re consistency

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense! I think the main concern is that the original JIRA ticket mentions that the "commit" language is what was causing confusion with the definition in the first place, so I think that could warrant overriding consistency in favor of clarity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, just made the change!

Copy link
Contributor

@FGasper FGasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything here is correct.

I wasn’t sure if we want to explain more of what happens in a commit—e.g., indexes become unique-ified, collection caps are reduced to production levels, etc.—but TBH I don’t know how much of that we consider to be implementation.

I’ll ping the team.

@ltran-mdb2
Copy link
Collaborator Author

Hi @FGasper ! I wanted to follow up on this and see if I should add anything else?

* - :ref:`Capped Collections <c2c-dr-capped-collections>`
- ``commit`` resets the required maximum size of capped collections
which ``mongosync`` set to the maximum allowable size on the
destination cluster.
Copy link
Contributor

@FGasper FGasper Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this table duplicate the information in the disaster-recovery page?

If so, that seems like additional overhead and opportunity for confusion/inaccuracy, if those 2 pages now have to be kept manually in sync.

Is it possible to write the table out to a file, then include it separately? Maybe using rST’s include directive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The information in the commit table and the disaster recovery table is similar but has different use cases which is why I hesitate to make make them identical with an include file. For the disaster recovery page, the user needs to know which commands to use (for example, use the collMod command to set expireAfterSeconds to the desired value) to manually take action while the commit table does not need to provide that information. Because the disaster recovery table needs to include instructions on how to manually reset the changes as opposed to the commit table which is just describes how things happen automatically, I would be in favor of leaving the two tables separate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks!

Copy link
Contributor

@FGasper FGasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@sarah-olson-mongodb sarah-olson-mongodb merged commit 284c36d into mongodb:master Jan 10, 2024
ltran-mdb2 added a commit to ltran-mdb2/docs-cluster-to-cluster-sync that referenced this pull request Jan 11, 2024
* DOCSP-28714 clarifying commit

* DOCSP-28174 adding commit information

* DOCSP-28174 adding commit information

* DOCSP-28174 copy edits

* DOCSP-28174 copy edits

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 updating disaster recovery
sarah-olson-mongodb pushed a commit that referenced this pull request Jan 11, 2024
* DOCSP 28174 adding info to commit page (#222)

* DOCSP-28714 clarifying commit

* DOCSP-28174 adding commit information

* DOCSP-28174 adding commit information

* DOCSP-28174 copy edits

* DOCSP-28174 copy edits

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 adding commit behavior table

* DOCSP-28174 updating disaster recovery

* DOCSP-28174-v1.7-backport removing capped collections reference
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants