-
Notifications
You must be signed in to change notification settings - Fork 22
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
DOCSP-48670-update-oracle-prereqs #247
base: master
Are you sure you want to change the base?
DOCSP-48670-update-oracle-prereqs #247
Conversation
✅ Deploy Preview for docs-relational-migrator ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
@mmaville-mdb lgtm with suggestions. Cheers.
source/jobs/prerequisites/oracle.txt
Outdated
|
||
.. step:: Turn on archive logging | ||
|
||
a. To see if archive logging is already enabled, run the following query: |
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.
Consider "To examine if ..."
source/jobs/prerequisites/oracle.txt
Outdated
@@ -33,7 +33,14 @@ About this Task | |||
architecture, see `Overview of Container Databases and Pluggable Databases <https://oracle-base.com/articles/12c/multitenant-overview-container-database-cdb-12cr1>`__. | |||
- Some commands differ based on whether the database is single or | |||
multi-tenant. In a multi-tenant database, permissions must | |||
include the suffix ``CONTAINER=ALL``. | |||
include the suffix ``CONTAINER=ALL``. To check whether the database is |
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.
Consider "To examine whether..."
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.
have a few suggestions, but overall LGTM
@@ -33,7 +33,14 @@ About this Task | |||
architecture, see `Overview of Container Databases and Pluggable Databases <https://oracle-base.com/articles/12c/multitenant-overview-container-database-cdb-12cr1>`__. | |||
- Some commands differ based on whether the database is single or | |||
multi-tenant. In a multi-tenant database, permissions must | |||
include the suffix ``CONTAINER=ALL``. | |||
include the suffix ``CONTAINER=ALL``. To examine whether the database is | |||
multi-tenant or not, run the following SQL query: |
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.
personal take on the previous feedback (but take it with a pinch of salt): I would not phrase it like this
To examine whether the database is multi-tenant or not
for a simple check / confirmation. this is not an exploration or a detailed investigation but rather a very simple check "true / false"
i think "to check" would be a better fit here
|
||
.. step:: Turn on archive logging | ||
|
||
a. To see if archive logging is already enabled, run the following query: | ||
a. To examine if archive logging is already enabled, run the following query: |
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.
same as with multi-tenancy check, in my personal opinion, "to examine" is a very strong / fancy word here for a mere "true / false" check
|
||
.. step:: Set up user permissions | ||
|
||
This procedure applies to Oracle instances which **are |
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.
np: i am not really sure why there is an emphasis on **are hosted**
? if we were to add an emphasis, it would make sense to put it on **Amazon RDS**
, but given this entire section is already behind a tab, i think any more emphasis here is unnecessary
.. procedure:: | ||
:style: normal | ||
|
||
.. step:: Set up user permissions |
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.
np: it might make more sense to put the next sentence before the ordered step list, potentially in a "important" banner:
This procedure applies to Oracle instances which are hosted on Amazon RDS
- Set up user permissions
...
|
||
.. step:: Turn on archive logging | ||
|
||
a. To examine if archive logging is already enabled, run the following query: |
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.
np: i think to examine
is a strong word for a mere "true / false" check; my personal preference would be to use to check
instead
DESCRIPTION
Addressing backlog of updates for the migration requisites page for Oracle. A new tab contains the RDS specific instructions, with edits for all relevant feedback interlaced throughout the ticket. Both Snapshot Jobs and Continuous Jobs had minor edits per feedback.
STAGING
Configure Migration Prerequisites for Oracle - Steps:
JIRA
https://jira.mongodb.org/browse/DOCSP-48670
BUILD LOG
Self-Review Checklist
External Review Requirements
What's expected of an external reviewer?