Improve user feedback on upload error - #3
Open
joselfrias wants to merge 3 commits into
Open
Conversation
… to more friendly user messages, adjust alchemy settings to try to prevent django from using stale connections
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.
Motivation
Database exceptions reached end users as stack traces or as unrelated messages.
upload_data_to_tmp_tablewrapped everything inexcept Exception: raise InvalidCSVFile("Error processing the file"), so a numeric overflow was reported as a malformed CSV and the original exception was discarded. Messages never said whether data had been saved or what to do next.Summary
postgres_errors.py(new) — classifies Postgres errors by SQLSTATE into who can fix them (USER/TRANSIENT), plusis_connection_lost(), which detects a dropped connection by the absence of a SQLSTATE on anOperationalError.errors.py(new) —UploadErrorbase carryingtitle,what_happened,data_saved,next_stepsand anincident_id. Copy is declared once per class instead of at each raise site.translate()sends the full traceback to the log and the curated object to the user. The eight exception classes previously inchecks.pynow live here.checks.py—upload_data_to_tmp_table→validate_data_in_existing_mat_views, split into load / probe / cleanup. Cleanup moved intofinally.updates.py— archive and insert wrapped separately, so a failure names which one it was.tasks.py— closes dead connections before writing status; previously the handler itself raisedInterfaceError, leaving uploads stuck inSTARTEDwith nothing shown in the UI. Switched to.filter().update(), sincedelete()clears the instance PK and a later rollback madesave()insert a duplicate.models.py—failure_message()renders the structured payload to HTML, falling back to the plain string for olderTaskResultrows.settings.py—pool_pre_ping,pool_recycleand TCP keepalives on the achilles engine, addressing theSSL SYSCALL error: EOF detectedfailures that started after Postgres moved to a remote host.Test plan
Existing Test Suite
uploader/tests.pypasses (imports moved tofile_handler.errors,FileChecksException→UploadError).File validation - each should show a title, cause and numbered next steps in the popover
.xlsxrenamed) → "This file couldn't be read as a CSV"count_value→ "Some values in the file aren't in the expected format"data_saved: truePostgres error classification
SUM()→FileDataCorrupted, notInvalidCSVFileConnection loss
SELECT pg_terminate_backend(<pid>)on the worker's connectionTemporaryFailure, notInvalidCSVFilePendingUploadreachesSTATE_FAILED(not stuck inSTARTED)TaskResultrow is written and the message renders in the UIupdate_achilles_results_data, then checkSELECT count(*) FROM achilles_results WHERE data_source_id = <id>—documents the partial-commit behaviour the retry work will need to fix
pool_pre_ping: run a query, kill the pooled connection frompsql, runanother — should succeed silently
Regression
UploadHistory, and deletes thePendingUploadachilles_results_tmpand its sequence are gone after both a successful and a failed runTaskResultrow (plain-string message) still renders readably