Skip to content
Snippets Groups Projects

Adding decompress_archive_file_timeout to application settings

2 unresolved threads

What does this MR do and why?

Currently the hardcoded timeout of 210seconds for decompressing archived files is too short for some customers. If we were to move this to application settings, self-managed users can set their own timeouts depending on how long they want to wait.

Screenshots or screen recordings

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

Before After
image.png image.png

How to set up and validate locally

  1. Ensure import/export still works.
  2. Timeout is changeable, by changing the timeout limit in the UI and then going into rails console to check Gitlab::CurrentSettings.current_application_settings.decompress_archive_file_timeout or the new database column decompress_archive_file_timeout should reflect the new changed timeout value

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 #421432 (closed)

Edited by Max Fan

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
  • Contributor
    3 Warnings
    :warning: b1345422: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: 7f39c2ed: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: Please add a merge request subtype to this merge request.
    2 Messages
    :book: This merge request adds or changes files that require a review from the Database team.
    :book: This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge.

    This merge request requires a database review. To make sure these changes are reviewed, take the following steps:

    1. Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
    2. Prepare your MR for database review according to the docs.
    3. Assign and mention the database reviewer suggested by Reviewer Roulette.

    The following files require a review from the Database team:

    • db/migrate/20230814181359_add_decompress_archive_file_timeout_to_application_setting.rb
    • db/schema_migrations/20230814181359
    • db/structure.sql

    Documentation review

    The following files require a review from a technical writer:

    The review does not need to block merging this merge request. See the:

    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 Piotr Skorupa current availability (@pskorupa) (UTC+2) Andy Soiron current availability (@Andysoiron) (UTC+2)
    database Charlie Ablett current availability (@cablett) (UTC+12) Andy Soiron current availability (@Andysoiron) (UTC+2)
    frontend Rajan Mistry current availability (@ramistry) (UTC+5.5) Himanshu Kapoor current availability (@himkp) (UTC+7)

    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

  • Contributor

    :warning: @mfanGitLab Some end-to-end (E2E) tests have been selected based on the stage label on this MR.

    Please start the trigger-omnibus-and-follow-up-e2e job in the qa stage and ensure the tests in follow-up-e2e:package-and-test-ee pipeline are passing before this MR is merged. (The E2E test pipeline is computationally intensive and we cannot afford running it automatically for all pushes/rebases. Therefore, this job must be triggered manually after significant changes at least once.)

    If you would like to run all E2E tests, please apply the pipeline:run-all-e2e label and trigger a new pipeline. This will run all tests in e2e:package-and-test pipeline.

    The E2E test jobs are allowed to fail due to flakiness. For the list of known failures please refer to the latest pipeline triage issue.

    Once done, please apply the :white_check_mark: emoji on this comment.

    For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.

  • Max Fan added 173 commits

    added 173 commits

    Compare with previous version

  • Max Fan added 1 commit

    added 1 commit

    Compare with previous version

  • A deleted user added Data WarehouseImpact Check label
  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :x: test report for af24c83f

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Manage      | 13     | 0      | 1       | 1     | 14    | ❗     |
    | Govern      | 34     | 0      | 0       | 0     | 34    | ✅     |
    | Create      | 36     | 2      | 0       | 2     | 38    | ❌     |
    | Plan        | 47     | 0      | 0       | 0     | 47    | ✅     |
    | Data Stores | 20     | 0      | 0       | 0     | 20    | ✅     |
    | Verify      | 8      | 0      | 0       | 0     | 8     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 158    | 2      | 1       | 3     | 161   | ❌     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :x: test report for 586b0b88

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Create           | 151    | 0      | 35      | 12    | 186   | ❗     |
    | Manage           | 158    | 1      | 12      | 27    | 171   | ❌     |
    | Configure        | 1      | 0      | 0       | 0     | 1     | ✅     |
    | Package          | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Plan             | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Data Stores      | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Monitor          | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Framework sanity | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Govern           | 4      | 0      | 0       | 0     | 4     | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 334    | 1      | 51      | 39    | 386   | ❌     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • Max Fan added 3 commits

    added 3 commits

    Compare with previous version

  • A deleted user added frontend label

    added frontend label

  • Max Fan resolved all threads

    resolved all threads

  • Contributor

    Database migrations (on the main database)

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).

    Migration Type Total runtime Result DB size change
    20230814181359 - AddDecompressArchiveFileTimeoutToApplicationSetting Regular 2.2 s :white_check_mark: +0.00 B
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 4
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230814181359 - AddDecompressArchiveFileTimeoutToApplicationSetting

    • Type: Regular
    • Duration: 2.2 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 18.7 ms 18.7 ms 18.7 ms 0
    ALTER TABLE "application_settings" ADD "decompress_archive_file_timeout" integer DEFAULT 210 NOT NULL
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    Histogram for AddDecompressArchiveFileTimeoutToApplicationSetting
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 4
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Other information

    No other migrations pending on GitLab.com

    Clone details
    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-2222216-10922352-main 2023-08-16T20:06:50Z 2023-08-16T16:17:51Z 2023-08-17 08:12:32 +0000
    database-testing-2222216-10922352-ci 2023-08-16T20:06:50Z 2023-08-12T15:50:05Z 2023-08-17 08:12:32 +0000

    Job artifacts

    Database migrations (on the ci database)

    1 Warnings
    :warning: 20230814181359 - AddDecompressArchiveFileTimeoutToApplicationSetting had a query that
    exceeded timing guidelines. Run time should not exceed 100ms, but it was 102.52ms. Please consider
    possible options to improve the query performance.
    ALTER TABLE "application_settings" ADD
    "decompress_archive_file_timeout" integer DEFAULT 210 NOT NULL

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).

    Migration Type Total runtime Result DB size change
    20230814181359 - AddDecompressArchiveFileTimeoutToApplicationSetting Regular 2.7 s :warning: +0.00 B
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 3
    0.1 seconds - 1 second 1
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    :warning: Migration: 20230814181359 - AddDecompressArchiveFileTimeoutToApplicationSetting

    • Type: Regular
    • Duration: 2.7 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 102.5 ms 102.5 ms 102.5 ms 0
    ALTER TABLE "application_settings" ADD "decompress_archive_file_timeout" integer DEFAULT 210 NOT NULL
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    Histogram for AddDecompressArchiveFileTimeoutToApplicationSetting
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 3
    0.1 seconds - 1 second 1
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Other information

    Other migrations pending on GitLab.com
    Migration Type Total runtime Result DB size change
    20230728171609 - AddCiJobAnnotationsPlanLimits Regular 3.0 s :white_check_mark: +0.00 B
    20230803125434 - AddHasMergeRequestOnVulnerabilityReadsTrigger Regular 2.7 s :white_check_mark: +0.00 B
    20230804064817 - BackfillGoogleCloudLoggingName Regular 2.8 s :white_check_mark: +0.00 B
    20230804065052 - AddNotNullToGcpConfigName Regular 2.5 s :white_check_mark: +0.00 B
    20230807101745 - AddActiveToAuditEventsStreamingHeaders Regular 2.6 s :white_check_mark: +8.00 KiB [note]
    20230807105131 - AddActiveToInstanceAuditEventsStreamingHeaders Regular 2.6 s :white_check_mark: +0.00 B
    20230808140338 - AddFluxResourceColumnToEnvironments Regular 3.4 s :white_check_mark: +0.00 B
    20230809165212 - AddPathPrefixAndBuildRefToPagesDeployments Regular 2.7 s :white_check_mark: +0.00 B
    20230809165213 - AddIndexToPathPrefixAndBuildRefToPagesDeployments Regular 4.2 s :white_check_mark: +8.00 KiB [note]
    20230809192256 - AddFileSizeLimitToPlanLimits Regular 2.6 s :white_check_mark: +0.00 B
    20230810132301 - AddHasRemediationsToVulnerabilityReads Regular 2.6 s :white_check_mark: +0.00 B
    20230814055259 - AddPipelineIdAndExportTypeToDependencyListExports Regular 2.6 s :white_check_mark: +0.00 B
    20230814055310 - AddIndexPipelineIdToDependencyListExports Regular 3.0 s :white_check_mark: +8.00 KiB [note]
    20230701053315 - EnsureAgainBackfillForCiPipelineVariablesPipelineIdIsFinished Post deploy 2.7 s :white_check_mark: +0.00 B
    20230712052619 - DropIndexDeploymentsOnProjectIdAndStatus Post deploy 3.6 s :white_check_mark: +0.00 B
    20230712054057 - DropIndexDeploymentsOnProjectIdSha Post deploy 3.5 s :white_check_mark: +0.00 B
    20230712055956 - DropIndexDeploymentsOnEnvironmentIdAndIidAndProjectId Post deploy 3.6 s :white_check_mark: +0.00 B
    20230726024322 - AddNotValidForeignKeyForCiPipelineVariablesPipelineId Post deploy 3.6 s :warning: +8.00 KiB [note]
    20230727102936 - DropPreparedAtIndex Post deploy 4.2 s :white_check_mark: +8.00 KiB [note]
    20230727103144 - AddPreparedAtCreatedAtIndex Post deploy 4.0 s :white_check_mark: +0.00 B
    20230727132342 - PrepareIndexOnVulnerabilityOccurrencesUuidAsync Post deploy 3.0 s :white_check_mark: +0.00 B
    20230728122928 - PrepareIndexOnVulnerabilityOccurrencesUuidIncludingVulnerabilityIdAsync Post deploy 2.7 s :white_check_mark: +0.00 B
    20230804053643 - AddTicketWorkItemType Post deploy 2.6 s :white_check_mark: +0.00 B
    20230804121704 - RemoveNamespacesUserDetailsEnterpriseGroupIdFk Post deploy 3.0 s :white_check_mark: +0.00 B
    20230804121705 - RemoveNamespacesUserDetailsProvisionedByGroupIdFk Post deploy 3.0 s :white_check_mark: +0.00 B
    20230809090349 - EnsureIdUniquenessForPCiBuildsV2 Post deploy 3.1 s :white_check_mark: +0.00 B
    20230809133249 - IndexSbomOccurrencesOnProjectIdComponentIdAndInputFilePath Post deploy 3.2 s :white_check_mark: +8.00 KiB [note]
    20230810122746 - EnsureSnippetUserMentionsBigintBackfillIsFinishedForSelfHosts Post deploy 2.5 s :white_check_mark: +0.00 B
    20230810123044 - SwapSnippetUserMentionsNoteIdToBigintForSelfHosts Post deploy 2.5 s :white_check_mark: +0.00 B
    20230811103654 - EnsureVumBigintBackfillIsFinishedForSelfHosts Post deploy 2.4 s :white_check_mark: +0.00 B
    20230811103941 - SwapVulnerabilityUserMentionsNoteIdToBigintForSelfHosts Post deploy 2.5 s :white_check_mark: +0.00 B
    Clone details
    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-2222216-10922352-main 2023-08-16T20:06:50Z 2023-08-16T16:17:51Z 2023-08-17 08:12:32 +0000
    database-testing-2222216-10922352-ci 2023-08-16T20:06:50Z 2023-08-12T15:50:05Z 2023-08-17 08:12:32 +0000

    Job artifacts


    Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic

  • Max Fan added 1 commit

    added 1 commit

    • 586b0b88 - Adding decompress_archive_file_timeout to application settings

    Compare with previous version

  • Max Fan changed the description

    changed the description

  • Max Fan changed milestone to %16.3

    changed milestone to %16.3

  • Max Fan changed milestone to %16.4

    changed milestone to %16.4

  • Max Fan marked this merge request as ready

    marked this merge request as ready

  • Max Fan added 248 commits

    added 248 commits

    Compare with previous version

    • Author Maintainer
      Resolved by Chad Woolley

      @mhamda can you help with the database review? There's some flaky specs that have been failing in this pipeline. Wondering if you've seen them around?

      An error occurred in an `after(:context)` hook.
      Failure/Error: connection.public_send(...)
      ActiveRecord::QueryCanceled:
        PG::QueryCanceled: ERROR:  canceling statement due to statement timeout
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
      # ./lib/gitlab/database/load_balancing/load_balancer.rb:133:in `block in read_write'
      # ./lib/gitlab/database/load_balancing/load_balancer.rb:204:in `retry_with_backoff'
      # ./lib/gitlab/database/load_balancing/load_balancer.rb:122:in `read_write'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:96:in `method_missing'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `block in write_using_load_balancer'
      # ./lib/gitlab/database/load_balancing/load_balancer.rb:133:in `block in read_write'
      # ./lib/gitlab/database/load_balancing/load_balancer.rb:204:in `retry_with_backoff'
      # ./lib/gitlab/database/load_balancing/load_balancer.rb:122:in `read_write'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:126:in `write_using_load_balancer'
      # ./lib/gitlab/database/load_balancing/connection_proxy.rb:96:in `method_missing'
      # ./spec/support/db_cleaner.rb:11:in `delete_from_all_tables!'
      # ./spec/rake_helper.rb:15:in `block (2 levels) in <top (required)>'
      # ------------------
      # --- Caused by: ---
      # PG::QueryCanceled:
      #   ERROR:  canceling statement due to statement timeout
      #   ./lib/gitlab/database/load_balancing/connection_proxy.rb:127:in `public_send'
  • Max Fan requested review from @carlad-gl, @mfluharty, @eread, and @mhamda

    requested review from @carlad-gl, @mfluharty, @eread, and @mhamda

  • Evan Read
  • Evan Read removed review request for @eread

    removed review request for @eread

  • Mohamed Hamda approved this merge request

    approved this merge request

  • Mohamed Hamda removed review request for @mhamda

    removed review request for @mhamda

  • Mohamed Hamda requested review from @l.rosa

    requested review from @l.rosa

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

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • added databasereviewed label and removed databasereview pending label

  • Miranda Fluharty approved this merge request

    approved this merge request

  • Miranda Fluharty requested review from @jivanvl and removed review request for @mfluharty

    requested review from @jivanvl and removed review request for @mfluharty

  • Jose Ivan Vargas approved this merge request

    approved this merge request

  • Jose Ivan Vargas removed review request for @jivanvl

    removed review request for @jivanvl

  • Carla Drago approved this merge request

    approved this merge request

  • Carla Drago requested review from @cwoolley-gitlab and removed review request for @carlad-gl

    requested review from @cwoolley-gitlab and removed review request for @carlad-gl

  • Leonardo da Rosa
  • @mfanGitLab, great work! I left only a small comment for you to take a look⁣ at :thumbsup:

  • Max Fan added 421 commits

    added 421 commits

    Compare with previous version

  • 11930 11930 sentry_clientside_traces_sample_rate double precision DEFAULT 0.0 NOT NULL,
    11931 11931 protected_paths_for_get_request text[] DEFAULT '{}'::text[] NOT NULL,
    11932 11932 max_decompressed_archive_size integer DEFAULT 25600 NOT NULL,
    11933 ci_max_total_yaml_size_bytes integer DEFAULT 157286400 NOT NULL,
    11934 11933 prometheus_alert_db_indicators_settings jsonb,
    11934 ci_max_total_yaml_size_bytes integer DEFAULT 157286400 NOT NULL,
    • suggestion (non-blocking) looks like this line was inadvertently moved by the migration - can we move it back?

    • Author Maintainer

      Good callout! This was actually changed because I ran ./scripts/regenerate-schema .

      I don't think this was in the right spot in the beginning? Reasoning: New columns are added at the bottom: https://docs.gitlab.com/ee/development/migration_style_guide.html#schema-changes

      prometheus_alert_db_indicators_settings jsonb, was renamed in migrate/20230808123101_rename_application_settings_database_apdex_settings.rb

      ci_max_total_yaml_size_bytes integer DEFAULT 157286400 NOT NULL, was added in migrate/20230808135706_add_max_yaml_size_to_application_settings.rb

      So ci_max_total... should be below prometheus_alert on structure.sql? Can you help confirm @l.rosa :pray: and re-approve if it looks good?

      Edited by Max Fan
    • @mfanGitLab after rebasing with master and regenerating the schema with bin/rails db:reset, this is the version of the schema I got:

      max_decompressed_archive_size integer DEFAULT 25600 NOT NULL,
      -prometheus_alert_db_indicators_settings jsonb,
      ci_max_total_yaml_size_bytes integer DEFAULT 157286400 NOT NULL,
      +prometheus_alert_db_indicators_settings jsonb,
      decompress_archive_file_timeout integer DEFAULT 210 NOT NULL
    • Author Maintainer

      Thank you for double checking! @l.rosa :pray: . Rebasing off master moved it! I'm a bit confused though, do you know why the ci_max_total_yaml is before prometheus even though the migration happens after?

      CC: @cwoolley-gitlab as the approval was lost :sweat_smile:

    • @mfanGitLab From Postgres docs:

      Postgres currently defines column order based on the attnum column of the pg_attribute table. The only way to change column order is either by recreating the table, or by adding columns and rotating data until you reach the desired layout.

      Let's take a look at the table order in the DB, in the master branch:

      SELECT
        table_information.relname  AS table_name,
        col_information.attname AS column_name,
        col_information.attnum AS column_order
      FROM pg_attribute AS col_information
      JOIN pg_class     AS table_information  ON col_information.attrelid = table_information.oid
      JOIN pg_namespace AS schema_information ON table_information.relnamespace = schema_information.oid
      WHERE table_information.relkind IN ('r', 'p')
      AND schema_information.nspname IN ('public')
      AND table_information.relname = 'application_settings'
      AND col_information.attname IN ('max_decompressed_archive_size', 'ci_max_total_yaml_size_bytes', 'prometheus_alert_db_indicators_settings', 'decompress_archive_file_timeout')
      ORDER BY col_information.attnum;

      After resetting the db, I'm getting the following column order:

      table_name column_name column_order
      application_settings max_decompressed_archive_size 519
      application_settings ci_max_total_yaml_size_bytes 520
      application_settings prometheus_alert_db_indicators_settings 521

      Then, after switching to this branch and performing an bin/rails db:migrate, I'm getting:

      table_name column_name column_order
      application_settings max_decompressed_archive_size 519
      application_settings ci_max_total_yaml_size_bytes 520
      application_settings prometheus_alert_db_indicators_settings 521
      application_settings decompress_archive_file_timeout 522

      In the development cycle, column order is more based on the order that migrations reach master (and change the schema), rather than migration order itself. In this case, the schema was changed before. If we recreate the DB using migration order, then, the column order will be different:

      bin/rails db:drop db:create db:migrate

      table_name column_name column_order
      application_settings max_decompressed_archive_size 546
      application_settings prometheus_alert_db_indicators_settings 548
      application_settings ci_max_total_yaml_size_bytes 549
      application_settings decompress_archive_file_timeout 550

      I believe something happened in this MR, as the column order doesn't seem right. My guess is:

      • MR was opened some time ago;
      • 20230808123136_cleanup_application_settings_database_apdex_settings_rename.rb was created;
      • 20230808135706_add_max_yaml_size_to_application_settings.rb was created, local db was migrated;
      • 20230808123136_cleanup_application_settings_database_apdex_settings_rename.rb was merged to master;
      • MR was rebased, but, at the local db, 20230808135706 was applied first than 20230808123136;
      • When the dump was created, the column order was swapped;

      So, it seems that ci_max_total_yaml_size_bytes was not placed in the correct order in the schema. I'll tag the MR author to take a look :thumbsup:

      /cc @Kasia_Misirli

    • I have not realised that anything was out of place. What are the recommended steps? :thinking:

    • Please register or sign in to reply
  • Chad Woolley
  • Chad Woolley approved this merge request

    approved this merge request

  • @mfanGitLab This looks good. I left a couple of non-blocking suggestions for cleanup, but I'll go ahead and approve it.

    I'll wait to merge to let you address those suggestions if you want. Ping me again when it's ready for merge.

  • Max Fan added 383 commits

    added 383 commits

    • cf05808e...831cad01 - 379 commits from branch master
    • eae6ac15 - Adding decompress_archive_file_timeout to application settings
    • 7f39c2ed - Adding disable_dd_transactions
    • b1345422 - Running regenerate-schema
    • af24c83f - Moving prometheus_alert_db_indicators_settings down one

    Compare with previous version

  • mentioned in issue #422757 (closed)

  • Chad Woolley resolved all threads

    resolved all threads

  • Chad Woolley approved this merge request

    approved this merge request

  • Chad Woolley requested review from @mhamda and @carlad-gl and removed review request for @cwoolley-gitlab and @l.rosa

    requested review from @mhamda and @carlad-gl and removed review request for @cwoolley-gitlab and @l.rosa

  • Leonardo da Rosa approved this merge request

    approved this merge request

  • Leonardo da Rosa resolved all threads

    resolved all threads

  • Chad Woolley resolved all threads

    resolved all threads

  • merged

  • @cwoolley-gitlab, did you forget to run a pipeline before you merged this work? Based on our code review process, if the latest pipeline was created more than 6 hours ago OR finished more than 2 hours ago, you should:

    1. Ensure the merge request is not in Draft status.
    2. Start a pipeline (especially important for Community contribution merge requests).
    3. Set the merge request to auto-merge.

    This is a guideline, not a rule. Please consider replying to this comment for transparency.

    This message was generated automatically. You're welcome to improve it.

    • Resolved by Max Fan

      Hello @mfanGitLab :wave:

      The database team is looking for ways to improve the database review process and we would love your help!

      If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:

      @gitlab-org/database-team

      And someone will be by shortly!

      Thanks for your help! :heart:

      This message was generated automatically. You're welcome to improve it.

  • Chad Woolley mentioned in commit 81d30c9d

    mentioned in commit 81d30c9d

  • Max Fan resolved all threads

    resolved all threads

  • added workflowstaging label and removed workflowcanary label

    • @mfanGitLab I'm currently running into a small issue in my GDK on the master branch. When running scripts/regenerate-schema, the command fails with

      NoMethodError: undefined method `decompress_archive_file_timeout' for #<ApplicationSetting id: 1 [...]`.

      It is very likely that my GDK is borked for some other reason as I haven't updated it in a month. I thought I would drop a note here anyways before I resume my investigation in case this is in fact related to this MR.

      Edited by Paul Gascou-Vaillancourt
    • @pgascouvaillancourt @mfanGitLab I just ran into this as well. I think the problem is that some application setting is getting loaded in order to perform the migration, so the validation breaks any migration that runs prior to the column has been added.

    • If you comment out the validation on decompress_archive_file_timeout then the migrations run

    • Author Maintainer

      @hfyngvason @pgascouvaillancourt do you get the same error if you do the db:migrate first and then do the regenerate-schema?

    • @mfanGitLab Good theory. It runs fine for all subsequent invocations, so maybe that would have helped. Possibly RAILS_ENV=test bin/rails db:migrate might be required, given that scripts/regenerate-schema uses the test db. But I haven't attempted any more elaborate reproduction.

      I think it is this line of code, getting invoked as part of rake db:reset.

    • Author Maintainer

      Yeah I think that's right. It looks like you need to have the migrations up-to-date first before you can try to regenerate-schema. The script only throws if ApplicationSettings are touched (which this MR does) so it's not very noticeable.

      I think there's a bunch of edge case regenerate-schema doesn't consider, that's why it was removed from ci :laughing: !31101 (merged) and hasn't been touched in a couple of years.

    • Please register or sign in to reply
  • Magdalena Frankiewicz
  • Max Fan mentioned in merge request !130229 (merged)

    mentioned in merge request !130229 (merged)

  • Mohamed Hamda removed review request for @mhamda

    removed review request for @mhamda

  • added sectioncore platform label and removed sectiondev label

  • Please register or sign in to reply
    Loading