Page MenuHomePhabricator

Store rejection reasons as a set in Cassandra
Closed, ResolvedPublic

Description

While working on T306034: Write GrowthExperiments image recommendation user feedback to EventGate and [attempting to update the image-suggestions-feedback schema](https://gerrit.wikimedia.org/r/c/809150), we realized that rejection_reason currently accepts only a single value, when it should be a set.

That would be a breaking change for Cassandra and require some schema migration. This task is to track that work.

Event Timeline

As the description notes, this is a breaking change. That said, I only see a couple of items in the feedback table, presumably test values of some sort? If there is no user-facing client code that would break as a result of a schema change, I could drop the rejected_reason attribute, and add rejected_reasons as set<text>.

kostajh added a subscriber: Tgr.

As the description notes, this is a breaking change. That said, I only see a couple of items in the feedback table, presumably test values of some sort? If there is no user-facing client code that would break as a result of a schema change, I could drop the rejected_reason attribute, and add rejected_reasons as set<text>.

That sounds fine to me. @Tgr do you have concerns?

This patch would start sending rejection_reason with a single string; if you are going to make the schema change, then I can hold off on that patch and rework it to support rejected_reasons as a set.

@Tgr do you have concerns?

No, sounds good.

@Tgr do you have concerns?

No, sounds good.

@Eevans please go ahead with adding rejected_reasons as set<text> when it's convenient for you. Thanks!

@Tgr do you have concerns?

No, sounds good.

@Eevans please go ahead with adding rejected_reasons as set<text> when it's convenient for you. Thanks!

Sounds good; Let's shoot for tomorrow morning (US timezone) when we can do it during a lull in eqiad bootstraps (T307802).

Eevans claimed this task.

@Tgr do you have concerns?

No, sounds good.

@Eevans please go ahead with adding rejected_reasons as set<text> when it's convenient for you. Thanks!

Sounds good; Let's shoot for tomorrow morning (US timezone) when we can do it during a lull in eqiad bootstraps (T307802).

Ok, this is now done.

FTR, in comments here I typo'd the name to be rejected_reason(s), when the current (now legacy) singular was rejection. The new name is the plural of what was there before, so rejection_reasons, sorry for any confusion.

Change 857039 had a related patch set uploaded (by Eevans; author: Eevans):

[generated-data-platform/datasets/image-suggestions@main] cassandra_schema.cql: change type of rejection reason(s)

https://gerrit.wikimedia.org/r/857039

Change 857039 merged by Eevans:

[generated-data-platform/datasets/image-suggestions@main] cassandra_schema.cql: change type of rejection reason(s)

https://gerrit.wikimedia.org/r/857039