Add push events to Geo event log

TODO

• Design the event log schema for the primary
• Add the event generation for push events

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #2384 (closed)

marked as a Work In Progress

changed the description

@stanhu @rspeicher @brodock @yorickpeterse Could you do the first review here? I'm still working on event generation for push events, but I want to get some early feedback on the event log schema. I tried to avoid usage of:

• polymorphic tables using push_event_id column
• JSON column to store event metadata using a separate table for every event
• updated_at column since events should not be updated

Using https://gitlab.com/gitlab-org/gitlab-ce/issues/31806 as a reference, this looks like a good schema to start.

A side point, I'm wondering now whether we should document best practices for SQL designs to avoid the pitfalls with the events table.

assigned to @dbalexandre

added 484 commits

• 54525b7e...211679e2 - 480 commits from branch master
• 5cf92a95 - Create geo_push_events table
• 4b3ab318 - Add GeoPushEvent model
• 0cd7d6af - Create geo_event_log table
• ecf83ec8 - Add GeoEventLog model

Compare with previous version

resolved all discussions

mentioned in issue #2506

@yorickpeterse thanks for the feedback. I made proposed changes/commented where needed.

added 40 commits

• a531f7a9...534c70ea - 30 commits from branch master
• aca2e6cb - Create geo_push_events table
• 5565f97d - Add GeoPushEvent model
• 763ac15b - Create geo_event_log table
• 261aa72a - Add GeoEventLog model
• 98ab46c6 - Move models to Geo namespace
• 8308ac34 - Move push_event_id to the last on the geo_event_log table
• 6918fa67 - Make event_type column as smallint and move it to the end of the table
• e2390bdd - Move foreign_key creation to create_table statement on CreateGeoEventLog
• d3b66cbd - Add metadata do geo_push_events table
• 46f71be0 - Create a push event on Geo event log when user push to the repository

Compare with previous version

@rspeicher @stanhu Could you do another round of review, please?

changed the description

marked the checklist item Add the event generation for push events as completed

resolved all discussions

marked the checklist item Changelog entry added, if necessary as completed

marked the checklist item Added for this feature/bug as completed

marked the checklist item Conform by the style guides as completed

unmarked as a Work In Progress

changed the description

@rspeicher @yorickpeterse Can you review, please?

@yorickpeterse Variable length fields aren't necessairly a pointer to some place else. We only use a pointer when the value to be stored in the column is too large to fit on the page after being compressed (and it's large enough for compression/TOAST'ing to be considered, with is 2kb).

What this tends to mean in practice is that it's best to push variable length fields to the end of the table because you can't be sure how wide they'll end up being and so there's no way to be 100% sure that you won't end up with a hole between the end of the variable-length structure at the next field which requires alignment.

Boolean fields are 1 byte, that's correct. I agree that using 'text' is better than 'varchar(255)', unless there is a need to limit the length of the field (in which case, I'd suggest that there be consideration for what the max length really should be and try to avoid defaults like '255').

In the above, simply moving "ref" to the end looks like it would result in a tuple with zero bytes of padding.

I strongly encourage making sure that FK relationships are between data types of the same type. Having a 32bit integer point to a 64bit integer wouldn't be good, nor the opposite.

@sfrost Ah, that's good to know. Thanks!

@dbalexandre in that case the ref field should be at the end.

marked the checklist item Conform by the merge request performance guides as completed

marked the checklist item Branch has no merge conflicts with master (if it does - rebase it please) as completed

changed the description

@yorickpeterse @rspeicher thanks for the review. I made proposed changes/commented where needed. Can you have another look?

added 380 commits

• a1a1c208...0a65e0da - 359 commits from branch master
• d4d11f94 - Create geo_push_events table
• 262ffb36 - Add GeoPushEvent model
• 1bf80eee - Create geo_event_log table
• 1e336c0c - Add GeoEventLog model
• df9ab045 - Move models to Geo namespace
• 2ce061dd - Move push_event_id to the last on the geo_event_log table
• a6a1deae - Make event_type column as smallint and move it to the end of the table
• 5cd0adbe - Move foreign_key creation to create_table statement on CreateGeoEventLog
• e8542f84 - Create a push event on Geo event log when repository is being updated
• 4e5c0e53 - Use 8 bytes for primary key values
• 216e3a7f - Create a push event on Geo event log when wiki is being updated
• 572ce9a3 - Rename Geo::PushService to Geo::PushEventStore
• 254a48ca - Create a push event on Geo event log when wiki is updated through UI
• 3d3d2aa3 - Add CHANGELOG
• e73d8855 - Re-use the constants to set the enum values on the push event model
• fb01c2a6 - Fix typo on the spec file for the push event store
• 8d164941 - Refactor wiki page services to avoid merge conflicts
• aaac49da - Reorder database columns on create geo_push_events migration
• 10bbcec6 - Use 8 bytes for the push_event_id foreign key on geo_event_log table
• ad6f6a71 - Refactor PostReceive worker to limit merge conflicts
• 1aba42d3 - Rename Geo::PushEvent to Geo::RepositoryUpdatedEvent

Compare with previous version

added 38 commits

• d8407bbc...6094bc77 - 15 commits from branch master
• b62b1c9f - Create geo_push_events table
• f66b0eac - Add GeoPushEvent model
• cb6c7cbe - Create geo_event_log table
• 846106a0 - Add GeoEventLog model
• b62da269 - Move models to Geo namespace
• ca40ebf1 - Move push_event_id to the last on the geo_event_log table
• 4a2774d4 - Make event_type column as smallint and move it to the end of the table
• 2ba6ea60 - Move foreign_key creation to create_table statement on CreateGeoEventLog
• 7354e4ab - Create a push event on Geo event log when repository is being updated
• 6be81a71 - Use 8 bytes for primary key values
• e48bba6f - Create a push event on Geo event log when wiki is being updated
• d8bd743e - Rename Geo::PushService to Geo::PushEventStore
• e0be98cf - Create a push event on Geo event log when wiki is updated through UI
• f48d447a - Add CHANGELOG
• 7c8a5c75 - Re-use the constants to set the enum values on the push event model
• ac2d7687 - Fix typo on the spec file for the push event store
• 1eef9e4d - Refactor wiki page services to avoid merge conflicts
• d61f2a56 - Reorder database columns on create geo_push_events migration
• a8b433cb - Use 8 bytes for the push_event_id foreign key on geo_event_log table
• 5f28791b - Refactor PostReceive worker to limit merge conflicts
• 71cc57b1 - Rename Geo::PushEvent to Geo::RepositoryUpdatedEvent
• 83748771 - Add bigserial as a native database type for MySQL adapter
• cc7aac36 - Fix Rubocop offenses

Compare with previous version

resolved all discussions

added 2 commits

• 3913bf55 - Fix EE::WikiPages::BaseService
• f51a4bd0 - Fix 'Every Sidekiq Worker' spec

Compare with previous version

added 1 commit

• cc1c6d10 - Fix 'Every Sidekiq Worker' spec

Compare with previous version

mentioned in merge request !1988 (merged)

@rspeicher @yorickpeterse Can you review, please?

assigned to @rspeicher

added 3 commits

• fa574f9f - Reorder database columns on create geo_repository_updated_events
• 07a564ed - Remove unnecessary index on created_at column on geo_event_log table
• a05acce8 - Use spec_helper rather than rails_helper on Geo model specs

Compare with previous version

added 3 commits

• fa574f9f - Reorder database columns on create geo_repository_updated_events
• 07a564ed - Remove unnecessary index on created_at column on geo_event_log table
• a05acce8 - Use spec_helper rather than rails_helper on Geo model specs

Compare with previous version

resolved all discussions

@dbalexandre LGTM, just a few more minor questions.

assigned to @dbalexandre

added 2 commits

• b007a1d4 - Fix small typo on Geo::RepositoryUpdatedEventStore
• 9043e664 - Fix small typo on EE::WikiPages::BaseService

Compare with previous version

@rspeicher Thanks I've addressed your comments. Let me know if there's anything else you'd want to change.

assigned to @rspeicher

resolved all discussions

approved this merge request

mentioned in commit 9100ee60

merged

mentioned in issue #846

mentioned in merge request !2807 (merged)