Skip to content
Snippets Groups Projects

Application limit for ES indexed field length

Merged Dylan Griffith requested to merge 201826-es-field-length-limit into master
All threads resolved!

What does this MR do?

Using the plan_limit table we can down store a limit for the maximum size of any fields being indexed in Elasticsearch. This is for #201826 (closed) . Any strings above the length limit will be truncated down to the limit.

We default to 0 which means unlimited so this has no affect for self-managed customers. On GitLab.com we will likely want to set this to around 20k as a sensible upper limit on what we should store.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

#201826 (closed)

Edited by Thong Kuah

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
  • 1 Message
    :book: This merge request adds or changes files that require a review from the Database team.

    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/20200204070729_add_elasticsearch_indexed_field_length_limit_to_application_settings.rb
    • db/schema.rb

    Reviewer roulette

    Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

    To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines.

    Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

    Category Reviewer Maintainer
    database Steve Abrams (@sabrams) Toon Claes (@toon)
    backend Ross Fuhrman (@rossfuhrman) Thong Kuah (@tkuah)
    frontend Jay Swain (@jayswain) Natalia Tepluhina (@ntepluhina)
    test Quality for spec/features/* Grant Young (@grantyoung) No maintainer available

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • Dmitry Gruzd approved this merge request

    approved this merge request

  • unassigned @dgruzd

  • @sabrams would you mind reviewing DB migration here.

    @tkuah would you mind doing backend maintainer review please.

  • Dylan Griffith added 1 commit

    added 1 commit

    • 2d178b81 - Application limit for ES indexed field length

    Compare with previous version

  • assigned to @sabrams and @tkuah

  • mentioned in issue #201805 (closed)

  • Steve Abrams
  • Steve Abrams assigned to @DylanGriffith and unassigned @sabrams

    assigned to @DylanGriffith and unassigned @sabrams

  • assigned to @sabrams and unassigned @DylanGriffith, @tkuah, and @rdickenson

  • Steve Abrams approved this merge request

    approved this merge request

  • added databasereviewed label and removed databasereview pending label

  • Steve Abrams assigned to @toon and unassigned @sabrams

    assigned to @toon and unassigned @sabrams

  • Dylan Griffith resolved all threads

    resolved all threads

  • Thong Kuah
  • Thong Kuah
  • Thanks @DylanGriffith - the specs look great - just a question about how plans work.

  • Thong Kuah assigned to @DylanGriffith and unassigned @tkuah

    assigned to @DylanGriffith and unassigned @tkuah

  • assigned to @tkuah and unassigned @DylanGriffith, @toon, and @rdickenson

  • assigned to @rdickenson and @toon

  • Dylan Griffith added 1 commit

    added 1 commit

    • 9bde964f - Application limit for ES indexed field length

    Compare with previous version

  • Dylan Griffith assigned to @mayra-cabrera and unassigned @toon

    assigned to @mayra-cabrera and unassigned @toon

  • @DylanGriffith Thanks, please see my response above.

  • Thong Kuah assigned to @DylanGriffith and unassigned @tkuah and @rdickenson

    assigned to @DylanGriffith and unassigned @tkuah and @rdickenson

  • Dylan Griffith added 1 commit

    added 1 commit

    • d31b23c1 - Application limit for ES indexed field length

    Compare with previous version

  • Dylan Griffith added 647 commits

    added 647 commits

    Compare with previous version

  • Dylan Griffith added 1 commit

    added 1 commit

    • d1c3ecd5 - Application limit for ES indexed field length

    Compare with previous version

  • Mayra Cabrera approved this merge request

    approved this merge request

  • added databaseapproved label and removed databasereviewed label

  • assigned to @DylanGriffith and unassigned @mayra-cabrera

  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thanks @DylanGriffith - some minor questions otherwise I'm happy to approve!

  • Dylan Griffith added 1 commit

    added 1 commit

    • 2c2c823d - Application limit for ES indexed field length

    Compare with previous version

  • Dylan Griffith resolved all threads

    resolved all threads

  • @tkuah back to you now with a couple of changes.

  • Dylan Griffith assigned to @tkuah and unassigned @DylanGriffith

    assigned to @tkuah and unassigned @DylanGriffith

  • Thong Kuah approved this merge request

    approved this merge request

  • LGTM! Thanks @DylanGriffith :100:

  • Thong Kuah enabled an automatic merge when the pipeline for 2c2c823d succeeds

    enabled an automatic merge when the pipeline for 2c2c823d succeeds

  • @DylanGriffith - Looks like https://gitlab.com/gitlab-org/gitlab/-/jobs/431089259 is a real failure. The other two IIRC are master failures, since fixed. Perhaps a rebase will resolve those

  • Thong Kuah assigned to @DylanGriffith and unassigned @tkuah

    assigned to @DylanGriffith and unassigned @tkuah

  • Dylan Griffith aborted the automatic merge because source branch was updated

    aborted the automatic merge because source branch was updated

  • Dylan Griffith added 1 commit

    added 1 commit

    • a3aae17f - Application limit for ES indexed field length

    Compare with previous version

  • @tkuah thanks for spotting that. Yes I forgot to add the translations. I've also rebased now.

  • Dylan Griffith assigned to @tkuah and unassigned @DylanGriffith

    assigned to @tkuah and unassigned @DylanGriffith

  • Dylan Griffith added 177 commits

    added 177 commits

    Compare with previous version

  • marked the checklist item Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. as completed

  • Thong Kuah enabled an automatic merge when the pipeline for d76b4c3b succeeds

    enabled an automatic merge when the pipeline for d76b4c3b succeeds

  • merged

  • Thong Kuah mentioned in commit e767eb5b

    mentioned in commit e767eb5b

  • Changzheng Liu mentioned in merge request !29728 (merged)

    mentioned in merge request !29728 (merged)

  • Dylan Griffith mentioned in merge request !36925 (merged)

    mentioned in merge request !36925 (merged)

  • added devopsdata stores label and removed devopssystems label

  • Please register or sign in to reply
    Loading