Soft-delete Submissions to reset student state - #33
Conversation
aa2520a to
36db7c0
Compare
There was a problem hiding this comment.
@mulby, I'm curious if this approach to soft-deleting will meet everyone's needs. By overriding the objects manager for the relevant classes, we'll essentially be hiding the deleted records from any query internal to the djangoapp.
When the data is gathered for analytics, is it accessed using django models (e.g. Submissions.objects.all()), or taken straight from the database using other methods? Is the same true for instuctor-initiated data downloads?
There was a problem hiding this comment.
When we export to researchers we just dump the data directly out of the database using a MySQL CLI so it won't use the model.
I'm not sure about the downloadable reports. @ormsbee might know?
There was a problem hiding this comment.
Thanks @mulby, that's very helpful.
I found the PR where @cahrens is working on the data downloads here(https://github.com/edx/edx-platform/pull/11167), and it's also using SQL instead of models, so I think we'll be covered on all fronts.
927db70 to
1d70b57
Compare
efischer19
changed the title
|
Sorry for coming late and asking a stupid question, but why is it necessary to delete their submission? It isn't enough to delete the |
|
That's actually a really good question @ormsbee - the impetus for this work is https://openedx.atlassian.net/browse/TNL-3880. The reason this is a bug that's only now appearing is staff tools - this block allows us to look up a submission for a student given a username, without checking the student's module state: https://github.com/edx/edx-ora2/blob/master/openassessment/xblock/staff_area_mixin.py#L338 Looking into it more just now, it seems that the reported buggy behavior only appears when using staff tools to "find" a "deleted" submission, this puts everything into a bad state (for the current staff user). However, refreshing the page gets everything back to where it needs to be. So I suppose we could do a simple fix, and have that staff access codepath also check to ensure the submission is valid. But then again, we may end up in this position again if there's a different place that tries to get a "deleted" submission, since it's only "deleted" insofar as the student module drops it's record. |
| cache_key = "submissions.submission.{}".format(sub.uuid) | ||
| cache.delete(cache_key) | ||
| sub.deleted = True | ||
| sub.save() |
There was a problem hiding this comment.
This cache clearing needs to happen no matter which of the 3 unlinking options we end up choosing
|
This is the SQL that will be run with the new status column migration: |
6b267dc to
cae1342
Compare
| name='status', | ||
| field=models.CharField(default=b'A', max_length=1, choices=[(b'D', b'Deleted'), (b'A', b'Active')]), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
@maxrothman (and FYI @scottrish), here's the timing data I got from my sandbox. 90 seconds to apply the migration to a table with 700,000 rows.
mysql> show create procedure stuff_submissions_tables;
+--------------------------+--------------------------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------------+----------------------+--------------------+
| Procedure | sql_mode | Create Procedure | character_set_client | collation_connection | Database Collation |
+--------------------------+--------------------------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------------+----------------------+--------------------+
| stuff_submissions_tables | STRICT_TRANS_TABLES,NO_ENGINE_SUBSTITUTION | CREATE DEFINER=`root`@`localhost` PROCEDURE `stuff_submissions_tables`(in numRows int)
begin
declare i int;
set i = 1;
start transaction;
while i < numRows do
insert into submissions_studentitem values(i, uuid(), uuid(), uuid(), "openassessment");
insert into submissions_submission values (i, uuid(), 1, now(), now(), concat(uuid(), uuid(), uuid()), i);
set i = i+1;
end while;
commit;
end | utf8 | utf8_general_ci | utf8_general_ci |
+--------------------------+--------------------------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------------+----------------------+--------------------+
1 row in set (0.00 sec)
mysql> call stuff_submissions_tables(700000);
Query OK, 0 rows affected (1 min 41.62 sec)
mysql> select count(*) from submissions_submission;
+----------+
| count(*) |
+----------+
| 699999 |
+----------+
1 row in set (0.14 sec)
mysql> exit
Bye
[efischer19-sandbox] efischer19@efischer19 i-6fd3e6ea:~$ time sudo -u www-data /edx/bin/python.edxapp /edx/bin/manage.edxapp lms --settings aws migrate submissions 0003
Operations to perform:
Target specific migration: 0003_submission_status, from submissions
Running migrations:
Rendering model states... DONE
Applying submissions.0003_submission_status... OK
real 1m29.847s
user 0m3.460s
sys 0m0.412s
There was a problem hiding this comment.
We're going to have to make a call here on feasibility (@e0d devops and @scottrish product).
Using the search linked below, we're looking at 500-1000 requests per hour, or on the order of tens per minute. @maxrothman and I have estimated that, given that request rate and the above timing data for a database not under load, we're looking at an upper bound of several hundred request failures during the migration. Is this an acceptable outage? It will only apply to users who are hitting the submissions database, which is only used by ORA and has a cache that will remain unaffected.
|
@maxrothman @e0d @scottrish Here's the data I got from running the new no-default migration. It's about a 50% reduction in time, so more like 2 minutes than 3 for production (probably). I've tested, and this does still require a table lock, causing errors for users making submissions during the migration. If we're not comfortable with this outage, we'll need to explore other options. |
|
Can we just add a new table, do a join, and only have values in that table On Wed, Feb 10, 2016 at 5:00 PM, Eric Fischer notifications@github.com
|
|
@efischer19 could you post the SQL the new migration runs? |
|
Sorry @maxrothman, meant to include that above. @ormsbee, we discussed your idea in planning this morning, and have decided that the amount of data isn't large enough to justify an extension table. From a product point of view, the small outage is acceptable. The plan is now:
|
|
@dianakhuang @robrap This PR is ready for review, can you give it a look when you get a chance? FYI @cahrens - this goes with the other 2 you've already reviewed, feel free to skip it if you're busy with Fedx stuff though. The problems with the global status message in https://openedx.atlassian.net/browse/TNL-4048 have been fixed via https://github.com/edx/edx-platform/pull/11525, which is going out in this week's release so that we can move forward with this release plan next week. |
| sub.save(update_fields=["status"]) | ||
|
|
||
| # Also clear out cached values | ||
| cache_key = "submissions.submission.{}".format(sub.uuid) |
There was a problem hiding this comment.
I would prefer just letting the 5 minute timeout take care of this.
There was a problem hiding this comment.
I'd have a function like get_submission_cache_key (or make... or create...), following whatever naming convention we've established for something like this. This function should be used all places the cache key is needed.
There was a problem hiding this comment.
Just to ensure we're clear - the individual submission cache (keyed on submissions.submission.{}) does not have a timeout, and needs to be handled here. The top scores cache is what would timeout, and I agree that it probably doesn't need to be explicitly handled here.
|
Looks good @efischer19. All of my comments are minor. Thanks. |
5bef451 to
3fc0558
Compare
7 participants
Previous methods of "deleting state" for a student had just issued
reset scores, so the (unscored) submission was still hanging around.
With this change, the submission can be "deleted" for all django-
relevant purposes, while still remaining in the database in case
it turns out to be interesting to analytics later.
Supersedes #32.
Sibling change to https://github.com/edx/edx-platform/pull/11371 and openedx/edx-ora2#862