Skip to content
Snippets Groups Projects

Add lease to update project statistics row and log concurrent updates

Merged Albert requested to merge 373595-lock-on-project-statistics-update into master

What does this MR do and why?

We need to be able to trace when there is potential concurrent update on a single project statistics row.

This MR makes the following changes:

  1. Refactor existing code to call a counter update method with a lock in ProjectStatistics.
  2. Wrap each attempt to update a project statistics with an exclusive lease lock.
  3. Log a warning each time it fails to obtain the lock and continue executing the update.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #373595

Edited by Albert

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Suggested Reviewers (beta)

    The individuals below may be good candidates to participate in the review based on various factors.

    You can use slash commands in comments to quickly assign /assign_reviewer @user1.

    Suggested Reviewers
    @rspeicher, @rymai, @smcgivern, @ayufan, @tkuah

    If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/357923.

    Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".

    Edited by GitLab Reviewer-Recommender Bot
  • A deleted user added backend label

    added backend label

  • 1 Warning
    :warning: Please add a merge request subtype to this merge request.
    1 Message
    :book: CHANGELOG missing:

    If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog.

    If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message.

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    backend Corinna Gogolok (@cwiesner) (UTC+2, 6 hours behind @alberts-gitlab) Pavel Shutsin (@pshutsin) (UTC+2, 6 hours behind @alberts-gitlab)

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Albert added 1 commit

    added 1 commit

    • 76354b9c - Lock project statistics for update

    Compare with previous version

  • Albert added 1 commit

    added 1 commit

    • 5ed89fca - Lock project statistics for update

    Compare with previous version

  • Albert added 1 commit

    added 1 commit

    • cde849ea - Lock project statistics for update

    Compare with previous version

  • Allure report

    allure-report-publisher generated test report!

    e2e-review-qa-blocking: :exclamation: test report for 63b34a4e

    expand test summary
    +-----------------------------------------------------------------------------------------+
    |                                     suites summary                                      |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    |                                    | passed | failed | skipped | flaky | total | result |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Plan                               | 47     | 0      | 1       | 46    | 48    | ❗     |
    | Verify                             | 12     | 0      | 1       | 10    | 13    | ❗     |
    | Create                             | 27     | 0      | 2       | 25    | 29    | ❗     |
    | Manage                             | 57     | 0      | 3       | 56    | 60    | ❗     |
    | Configure                          | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Package                            | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Version sanity check               | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Protect                            | 2      | 0      | 0       | 2     | 2     | ❗     |
    | Secure                             | 2      | 0      | 0       | 2     | 2     | ❗     |
    | Feature flag handler sanity checks | 9      | 0      | 0       | 0     | 9     | ✅     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Total                              | 156    | 0      | 10      | 141   | 166   | ❗     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+

    e2e-review-qa: :exclamation: test report for 64f6a1b2

    expand test summary
    +-----------------------------------------------------------------------------------------+
    |                                     suites summary                                      |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    |                                    | passed | failed | skipped | flaky | total | result |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Manage                             | 52     | 0      | 8       | 9     | 60    | ❗     |
    | Plan                               | 47     | 0      | 1       | 0     | 48    | ✅     |
    | Verify                             | 43     | 0      | 8       | 29    | 51    | ❗     |
    | Create                             | 30     | 0      | 1       | 2     | 31    | ❗     |
    | Protect                            | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Secure                             | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Package                            | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Feature flag handler sanity checks | 9      | 0      | 0       | 0     | 9     | ✅     |
    | Version sanity check               | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Configure                          | 0      | 0      | 1       | 0     | 1     | ➖     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+
    | Total                              | 185    | 0      | 21      | 40    | 206   | ❗     |
    +------------------------------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :exclamation: test report for 64f6a1b2

    expand test summary
    +---------------------------------------------------------------------------+
    |                              suites summary                               |
    +----------------------+--------+--------+---------+-------+-------+--------+
    |                      | passed | failed | skipped | flaky | total | result |
    +----------------------+--------+--------+---------+-------+-------+--------+
    | Create               | 320    | 0      | 10      | 10    | 330   | ❗     |
    | Plan                 | 114    | 0      | 0       | 0     | 114   | ✅     |
    | Protect              | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Verify               | 86     | 0      | 16      | 0     | 102   | ✅     |
    | Manage               | 178    | 0      | 28      | 6     | 206   | ❗     |
    | Secure               | 44     | 0      | 2       | 2     | 46    | ❗     |
    | Fulfillment          | 4      | 0      | 24      | 2     | 28    | ❗     |
    | Package              | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Release              | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Configure            | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Version sanity check | 0      | 0      | 2       | 2     | 2     | ➖     |
    | Analytics            | 4      | 0      | 0       | 0     | 4     | ✅     |
    +----------------------+--------+--------+---------+-------+-------+--------+
    | Total                | 762    | 0      | 94      | 22    | 856   | ❗     |
    +----------------------+--------+--------+---------+-------+-------+--------+
  • Albert added 1 commit

    added 1 commit

    • 1c6c8254 - Refactor legacy_increment_statistic to use update_counters

    Compare with previous version

  • Albert
  • Albert added 2 commits

    added 2 commits

    • 340fed6a - Consolidate counter update with lock into one method
    • cab35edb - Log a warning when unable to obtain a lock

    Compare with previous version

  • Albert changed title from Lock project statistics for update to Add lock for update to project statistics changes and log failed attempts to obtain lock

    changed title from Lock project statistics for update to Add lock for update to project statistics changes and log failed attempts to obtain lock

  • Albert changed the description

    changed the description

  • Albert marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • Albert added 1 commit

    added 1 commit

    • cc17c66d - Log a warning when unable to obtain a lock

    Compare with previous version

  • Albert added 1 commit

    added 1 commit

    Compare with previous version

  • Albert
  • Albert requested review from @vij

    requested review from @vij

  • Albert changed milestone to %15.4

    changed milestone to %15.4

  • Vijay Hawoldar
  • Vijay Hawoldar
  • Vijay Hawoldar
  • Vijay Hawoldar
  • Vijay Hawoldar
  • Vijay Hawoldar approved this merge request

    approved this merge request

  • Vijay Hawoldar requested review from @fabiopitino and removed review request for @vij

    requested review from @fabiopitino and removed review request for @vij

  • :wave: @vij, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.

    For more info, please refer to the following links:

  • Author Contributor

    Reminder to self:

    • Resolve conflict
    • Add feature flag on logging
    Edited by Albert
  • Albert added 503 commits

    added 503 commits

    • 63b34a4e...f70efa51 - 498 commits from branch master
    • c9c21e05 - Lock project statistics for update
    • 845b7d34 - Refactor legacy_increment_statistic to use update_counters
    • 37f382d0 - Consolidate counter update with lock into one method
    • d0404eba - Log a warning when unable to obtain a lock
    • 778eb9d4 - Remove unused method

    Compare with previous version

  • Albert added 5 commits

    added 5 commits

    • a2d57493 - Add feature flag to logging of lock retry
    • eea6da1a - Rename method
    • ad00e3a6 - Define reset_counter! on CounterAttribute
    • 83bff1e9 - Refactor to move locks into CounterAttribute
    • cd9ef417 - Add feature flag to counter attribute database lock

    Compare with previous version

  • A deleted user added feature flag label

    added feature flag label

  • Fabio Pitino
  • Fabio Pitino
  • Fabio Pitino removed review request for @fabiopitino

    removed review request for @fabiopitino

  • Albert added 408 commits

    added 408 commits

    • cd9ef417...dc364939 - 398 commits from branch master
    • 254b7363 - Lock project statistics for update
    • 82c05ffd - Refactor legacy_increment_statistic to use update_counters
    • 0a0ff230 - Consolidate counter update with lock into one method
    • ce709571 - Log a warning when unable to obtain a lock
    • 57113949 - Remove unused method
    • 711de620 - Add feature flag to logging of lock retry
    • 5d1bdea7 - Rename method
    • 765e713b - Define reset_counter! on CounterAttribute
    • 12eba858 - Refactor to move locks into CounterAttribute
    • 09034ed4 - Add feature flag to counter attribute database lock

    Compare with previous version

  • Albert added 3 commits

    added 3 commits

    • 02748089 - Reduce locking scope in `#refresh!`
    • f1b904e5 - Change methods to instance methods
    • 8b18fb6d - Change database update lock to an exclusive lease

    Compare with previous version

  • Albert added 2 commits

    added 2 commits

    • 53ebffb7 - Change methods to instance methods
    • e70fb36a - Change database update lock to an exclusive lease

    Compare with previous version

  • Albert added 1 commit

    added 1 commit

    • 5c0557bf - Combine feature flags into one

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading