Open
Conversation
DutChen18
requested changes
May 16, 2025
Member
DutChen18
left a comment
There was a problem hiding this comment.
All the validation routes should be examined for race conditions and extra error conditions.
We should also think about how the validator communicates invalid results. Maybe we require the validator to process results in order of submission (this seems like a good requirement anyways to get a consistent canonical result), and say that all results before submitted before the last valid result are invalid, and all results submitted after are not needed.
2823c25 to
a358ee8
Compare
f22bb0d to
fcec3cb
Compare
Open
Open
DutChen18
requested changes
May 19, 2025
bf05d47 to
70ba46b
Compare
DutChen18
requested changes
May 23, 2025
Member
There was a problem hiding this comment.
A few more things:
- There is still a race condition in
validate_submitthat will take more than just a transaction to solve. - I've been thinking a bit about how validators would solve the challenge of generating unique numbers for match ids. It's not that hard, they could just pick increasing numbers. But if we ever decide to store the match ids in the database, that will be a breaking change for validators because they now also have to worry about picking match ids that are consistent with those already in the db.
Maybe there's a better way: What if the match ids are the id of the assignment (or maybe result) with the first submitted result in that group. This gives groups a canonical match id that will always be the same even if recomputed. It also gives a fancy way of querying for "canonical assignments":SELECT * FROM assignments WHERE match_id = id, even in invalid groups. - We need to consider what assignments the validator must resend when a task is run through the validator for a second time. Does it only send assignments that were never sent before? Does it resend all
SubmittedandInconclusiveassignments? What about if the task is run through the validator again when it already has a canonical result id, does the validator need to resendValidandInvalidassignments as well? Right now it would, because the match ids are not stored in the database. With my suggestion for generating match ids, it wouldn't, even if the match ids are not stored in the database.
DutChen18
requested changes
May 24, 2025
16d8fba to
91cdc59
Compare
DutChen18
requested changes
Aug 6, 2025
DutChen18
reviewed
Aug 6, 2025
Maybe fully implemented? Implement a few suggestions Implement fixes Fix from rebasing, cargo fmt and clippy Move sqlx dependent stuff into a feature Refactoring, fixes Add invalid state Update names, fix logic in validate_fetch Fix query refactor and fix Renaming More progress Endpoints seem to work now Fix to work with latest master Add additional error state Rename errors Rename error Implement new error handling Cargo clippy/fmt Partial refactor - just for dutchen to work on api stuff Finish major refactor Remove old code Implement dutchen fixes where still applicable Fix problems post-rebase Major refactor - again Commments and small formatting touch-ups Rename enum states for clarity Small adjustmnets Refactor some more... Re-add bounds check for additional_assignments_needed Fix bounds check Update gitignore, move validate_fetch_request to its own request Remove unneeded error states Refactor Fix sqlx prepare after rebase Fix formatting
…gainst canonical result
e0e5a4c to
58e42b2
Compare
DutChen18
requested changes
Aug 24, 2025
2d647ba to
fde2af7
Compare
Member
|
There are still some things left to do:
|
Member
|
Member
|
Did 4 |
f26fa8e to
809a129
Compare
809a129 to
646948f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.