diff options
author | Mayra Cabrera <mcabrera@gitlab.com> | 2019-07-17 10:54:52 -0500 |
---|---|---|
committer | Mayra Cabrera <mcabrera@gitlab.com> | 2019-07-19 09:24:30 -0500 |
commit | d8315b9866a496a47871c77f232aa686c42345ed (patch) | |
tree | 6d00487c33411b5e32cb1503597c8c2c6f26edf7 /doc/development | |
parent | 0acbdf9c1c03648ee6aac7b771af62d345b21d64 (diff) | |
download | gitlab-ce-tc-database-review-process.tar.gz |
Resolves pending comments from reviewtc-database-review-process
- Addresses documentation feedback
- Refactor Danger methods
- Touches a file to activate Danger. Will remove after testing
Diffstat (limited to 'doc/development')
-rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 2 | ||||
-rw-r--r-- | doc/development/database_review.md | 72 |
2 files changed, 37 insertions, 37 deletions
diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index 2582bc10722..17328762c5b 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -104,7 +104,7 @@ from the [core team](https://about.gitlab.com/community/core-team/) or one of th [merge request coaches](https://about.gitlab.com/team/). When having your code reviewed and when reviewing merge requests, please keep the [code review guidelines](../code_review.md) in mind. And if your code also makes changes to the database, or does expensive queries, -check the [database review guidelines](../database_review.md) +check the [database review guidelines](../database_review.md). ### Keep it simple diff --git a/doc/development/database_review.md b/doc/development/database_review.md index e5caefca4ab..1413c2f69fb 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -8,14 +8,14 @@ practices for code review in general. A database review is required for: -* Changes that touch the database schema or perform data migrations, +- Changes that touch the database schema or perform data migrations, including files in: - * `db/` - * `lib/gitlab/background_migration/` -* Changes to the database tooling, e.g.: - * migration or ActiveRecord helpers in `lib/gitlab/database/` - * load balancing -* Changes that produce SQL queries that are beyond the obvious. It is + - `db/` + - `lib/gitlab/background_migration/` +- Changes to the database tooling, e.g.: + - migration or ActiveRecord helpers in `lib/gitlab/database/` + - load balancing +- Changes that produce SQL queries that are beyond the obvious. It is generally up to the author of a merge request to decide whether or not complex queries are being introduced and if they require a database review. @@ -34,25 +34,25 @@ for review. A Merge Request author's role is to: -* Decide whether a database review is needed. -* If database review is needed, add the ~database label. -* Use the [database changes](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md) +- Decide whether a database review is needed. +- If database review is needed, add the ~database label. +- Use the [database changes](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md) merge request template, or include the appropriate items in the MR description. A database **reviewer**'s role is to: -* Perform a first-pass review on the MR and suggest improvements to the author. -* Once satisfied, relabel the MR with ~"database::reviewed", approve it, and +- Perform a first-pass review on the MR and suggest improvements to the author. +- Once satisfied, relabel the MR with ~"database::reviewed", approve it, and reassign MR to the database **maintainer** suggested by Reviewer Roulette. A database **maintainer**'s role is to: -* Perform the final database review on the MR. -* Discuss further improvements or other relevant changes with the +- Perform the final database review on the MR. +- Discuss further improvements or other relevant changes with the database reviewer and the MR author. -* Finally approve the MR and relabel the MR with ~"database::approved" -* Merge the MR if no other approvals are pending or pass it on to +- Finally approve the MR and relabel the MR with ~"database::approved" +- Merge the MR if no other approvals are pending or pass it on to other maintainers as required (frontend, backend, docs). ### Distributing review workload @@ -70,32 +70,32 @@ make sure you have applied the ~database label and rerun the ### How to review for database -* Check migrations - * Review relational modeling and design choices - * Review migrations follow [database migration style guide](migration_style_guide.md), +- Check migrations + - Review relational modeling and design choices + - Review migrations follow [database migration style guide](migration_style_guide.md), for example - * [Check ordering of columns](ordering_table_columns.md) - * [Check indexes are present for foreign keys](migration_style_guide.md#adding-foreign-key-constraints) - * Ensure that migrations execute in a transaction or only contain + - [Check ordering of columns](ordering_table_columns.md) + - [Check indexes are present for foreign keys](migration_style_guide.md#adding-foreign-key-constraints) + - Ensure that migrations execute in a transaction or only contain concurrent index/foreign key helpers (with transactions disabled) - * Check consistency with `db/schema.rb` and that migrations are [reversible](migration_style_guide.md#reversibility) - * For data migrations, establish a time estimate for execution -* Check post deploy migration - * Make sure we can expect post deploy migrations to finish within 1 hour max. -* Check background migrations - * Review queries (for example, make sure batch sizes are fine) - * Establish a time estimate for execution -* Query performance - * Check for any obviously complex queries and queries the author specifically + - Check consistency with `db/schema.rb` and that migrations are [reversible](migration_style_guide.md#reversibility) + - For data migrations, establish a time estimate for execution +- Check post deploy migration + - Make sure we can expect post deploy migrations to finish within 1 hour max. +- Check background migrations + - Review queries (for example, make sure batch sizes are fine) + - Establish a time estimate for execution +- Query performance + - Check for any obviously complex queries and queries the author specifically points out for review (if any) - * If not present yet, ask the author to provide SQL queries and query plans + - If not present yet, ask the author to provide SQL queries and query plans (e.g. by using [chatops](understanding_explain_plans.md#chatops) or direct database access) - * For given queries, review parameters regarding data distribution - * [Check query plans](understanding_explain_plans.md) and suggest improvements + - For given queries, review parameters regarding data distribution + - [Check query plans](understanding_explain_plans.md) and suggest improvements to queries (changing the query, schema or adding indexes and similar) - * General guideline is for queries to come in below 100ms execution time - * If queries rely on prior migrations that are not present yet on production + - General guideline is for queries to come in below 100ms execution time + - If queries rely on prior migrations that are not present yet on production (eg indexes, columns), you can use a [one-off instance from the restore pipeline](https://ops.gitlab.net/gitlab-com/gl-infra/gitlab-restore/postgres-gprd) in order to establish a proper testing environment. |