-
Notifications
You must be signed in to change notification settings - Fork 6
Fix time-out issue in last "Index" stage #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Added a new test file `stage_index_scan_test.go` to validate the expected results of SQL queries against a SQLite database. - Introduced a map `expectedQueryResultMap` in `stage_index_scan.go` to store expected results for various queries, enhancing the test coverage and ensuring accuracy of query outputs.
- Replaced the use of `exec.Command` to copy the test database with `os.Symlink` for improved efficiency and clarity in the test setup process. - Updated error logging to reflect the change in method for creating the test database symlink.
@andy1li can you elaborate a bit more on how long each of these actions takes (copying vs. querying)? Which is the dominant factor? It's weird that this hasn't happened so far but is suddenly a problem. Wonder if I should contact fly to see if it's an issue on their end. |
@rohitpaulk Both copying and querying could time out (20s) on their own. An inconsistency is that user's code could query the db without any issue. Yeah, fly might provide more insights. Just FYI, here're some disk and memory usage info that I gathered during debugging (Nothing out of the ordinary):
|
- Changed the method for creating the test database link from `os.Symlink` to `os.Link` for improved functionality. - Updated error logging to reflect the change in method for creating the test database link.
Cool let's work with fly on this, definitely seems off. We'll focus on the copy issue first. @andy1li could you help me out here a bit please before I contact:
|
On it! Just to clarify:
@rohitpaulk Will our current time-out err msg suffice, or do I need something special here? ![]() |
10 runs completed: https://app-staging.codecrafters.io/courses/sqlite/admin/submissions ![]() Oops, it's more accurate to log |
@andy1li so all of them failed on cp exceeding the allotted time? cp never went through and caused the failure where it occasionally goes through quick and the next one causes failures? (Trying to ascertain if this is intermittent or not - it's harder to make a case with fly if it isn't intermittent) |
@rohitpaulk Yes, all 10 of my recent submissions look like this (cp never went through):
Furthermore, not a single submission (out of about a 100) showed signs of going through cp since July 24 (expect for a couple where I created a symlink to replace cp when debugging.) (For the user submissions, it's possible that cp went through but their own code hung before outputting anything. But it's impossible to tell for sure.) |
I've reached out to Fly.io about this with a repro: https://github.com/codecrafters-io/flyio-disk-performance-test |
Also pushed a commit here that uses symlinks for now: c2598c7 Checked major languages and all of them seem to follow symlinks |
There're two ways in which the tester can hang, both related to companies.db
cp
ing itFix / Workaround: