Skip to content
This repository was archived by the owner on Sep 10, 2021. It is now read-only.

Conversation

mgrauer
Copy link
Contributor

@mgrauer mgrauer commented Mar 4, 2016

@cpatrick PTAL.

The backend to setting up aggregate metric notifications. Some of it anyway.

CREATE INDEX "tracker_aggregate_metric_spec_branch" ON "tracker_aggregate_metric_spec" ("branch");

CREATE TABLE IF NOT EXISTS "tracker_user2aggregate_metric_spec" (
"id" serial PRIMARY KEY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is counter to our general convention of <table_name>_id.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the MySQL table have a primary key? If not, this is correct.

@cpatrick
Copy link
Contributor

cpatrick commented Mar 7, 2016

Other than the nits (which may be illegitimate as @jamiesnape suggests), LGTM.

@mgrauer
Copy link
Contributor Author

mgrauer commented Mar 7, 2016

Does the MySQL table have a primary key?

It has a primary compound key

PRIMARY KEY (`user_id`, `aggregate_metric_spec_id`)

Looking at this more carefully, I don't have the corresponding unique compound indices in pgsl and sqlite.

Perhaps the way forward is to

  • remove tracker_user2aggregate_metric_spec's primary compound key, make it a unique compound key instead (so as to uphold what @jamiesnape says about id in the other two dbs for when no primary key exists in mysql)
  • add a unique compound key to pgsql and sqlite

Thoughts?

@cpatrick
Copy link
Contributor

cpatrick commented Mar 7, 2016

I guess whatever we normally do in our user2* tables?

@mgrauer
Copy link
Contributor Author

mgrauer commented Mar 7, 2016

normally

Ahem.

I'll follow feed2community and make a consistency fix for user2group.

@mgrauer
Copy link
Contributor Author

mgrauer commented Mar 8, 2016

@cpatrick PTAL.

This changeset is small since your last checkpoint.

@cpatrick
Copy link
Contributor

cpatrick commented Mar 8, 2016

LGTM.

mgrauer pushed a commit that referenced this pull request Mar 8, 2016
…tification

User to aggregatemetric notification
@mgrauer mgrauer merged commit aa57e84 into master Mar 8, 2016
@mgrauer mgrauer deleted the user_to_aggregatemetric_notification branch March 8, 2016 19:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants