Implement disabling a judge #2005
Conversation
WallE256
force-pushed
the
disable-judge
branch
from
a5ac867
to
8ce779a
Compare
Codecov ReportBase: 46.69% // Head: 46.61% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2005 +/- ##
==========================================
- Coverage 46.69% 46.61% -0.08%
==========================================
Files 238 239 +1
Lines 13236 13322 +86
==========================================
+ Hits 6180 6210 +30
- Misses 7056 7112 +56
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
judge/models/runtime.py
Outdated
| def toggle_disabled(self): | ||
| self.is_disabled = not self.is_disabled | ||
| self.save(update_fields=['is_disabled']) | ||
| disconnect_judge(self, force=True) |
There was a problem hiding this comment.
Should this send a forced disconnect if you're re-enabling the judge?
In particular, it looks like this function is used less like toggle disable, and more like disable.
There was a problem hiding this comment.
The purpose of forcing a disconnect is to update the is_disabled cached property in the bridge, which helps to not access the Judge DB object for every submission and queue removal.
There was a problem hiding this comment.
I think there should be a bridge RPC added rather than hacking around it this way. The judge we run on dmoj.ca will auto-reconnect, but it's not guaranteed that all judges will.
There was a problem hiding this comment.
Added a bridge RPC instead of the disconnect method.
WallE256
force-pushed
the
disable-judge
branch
from
8ce779a
to
19716d8
Compare
WallE256
force-pushed
the
disable-judge
branch
from
19716d8
to
46fdc86
Compare
| <a style="display: none" title="{% trans "Disable" %}" href="{% url 'admin:judge_judge_disable' original.pk %}" | ||
| class="button disable-link"> | ||
| <i class="fa fa-lg fa-ban"></i> | ||
| <span class="text">{% trans "Disable" %}</span> |
There was a problem hiding this comment.
Don't know how I feel about this being a toggle but always showing Disable.
There was a problem hiding this comment.
This has been addressed.
WallE256
force-pushed
the
disable-judge
branch
2 times, most recently
from
02e5ff9
to
39d0d98
Compare
judge/bridge/django_handler.py
Outdated
| @@ -18,6 +18,7 @@ def __init__(self, request, client_address, server, judges): | |||
| 'submission-request': self.on_submission, | |||
| 'terminate-submission': self.on_termination, | |||
| 'disconnect-judge': self.on_disconnect_request, | |||
| 'disable-judge': self.update_disable_judge, | |||
There was a problem hiding this comment.
| 'disable-judge': self.update_disable_judge, | |
| 'disable-judge': self.on_disable_judge, |
for consistency
judge/models/runtime.py
Outdated
| @@ -147,6 +149,13 @@ def disconnect(self, force=False): | |||
|
|
|||
| disconnect.alters_data = True | |||
|
|
|||
| def toggle_disabled(self): | |||
| self.is_disabled = not self.is_disabled | |||
| self.save(update_fields=['is_disabled']) | |||
There was a problem hiding this comment.
I think this save should come after the RPC, so we don't end up (in case of an error) with a bridge that does not agree in state with the site.
| @@ -22,5 +23,18 @@ | |||
| <i class="fa fa-lg fa-plug"></i> | |||
| <span class="text">{% trans "Terminate" %}</span> | |||
| </a> | |||
| {% if not original.is_disabled %} | |||
| <a style="display: none" title="{% trans "Disable" %}" href="{% url 'admin:judge_judge_disable' original.pk %}" | |||
There was a problem hiding this comment.
Missing indentation within the if/else.
WallE256
force-pushed
the
disable-judge
branch
from
39d0d98
to
1dc5f98
Compare
This feature removes a disabled judge from the queue handling, such that normal or rejudged submissions will not be allocated to it for grading. However, it will still receive submissions intended for it specifically.