-
Notifications
You must be signed in to change notification settings - Fork 0
Now testing system works! #22
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses intermittent job status issues by refining thread termination logic, updating configuration and API messaging, and standardizing serialization tags.
- Replace "break" with "return" in thread loops to ensure proper goroutine termination.
- Update JSON/YAML tags and configuration fields for consistency.
- Refine error reporting and connector logic with improved context handling.
Reviewed Changes
Copilot reviewed 59 out of 61 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| invoker/runner.go | Changed thread exit from "break" to "return" for proper termination. |
| invoker/job_executor.go | Adjusted sandbox thread exit to use "return" instead of "break". |
| invoker/job.go | Updated ActiveJobs removal to avoid duplicate deletions and ensure locking. |
| invoker/invoker.go | Improved channel capacity, router grouping, and address formatting. |
| invoker/handler.go | Corrected error message text for invoker job parsing. |
| common/testing_system.go | Enhanced panic capture and shutdown logic with stack trace logging. |
| common/db/models/submission.go & problem.go | Standardized JSON/YAML tag case for API consistency. |
| common/constants/verdict/verdict.go | Removed unused verdict constants. |
| common/connectors/* | Updated context handling, error message clarity, and base URL usage. |
| common/config/db.go | Updated YAML tag naming and added InMemory support for testing. |
Files not reviewed (2)
- go.mod: Language not supported
- invoker/testdata/compiler/scripts/cpp.sh.tmpl: Language not supported
Comments suppressed due to low confidence (7)
common/constants/verdict/verdict.go:22
- The removal of 'QD' and 'CL' verdict constants could affect downstream logic if they are still referenced; ensure that any related code has been updated accordingly.
- QD Verdict = "QD" // Queued
common/config/db.go:4
- Changing the YAML tag for DSN to a capitalized form requires corresponding updates in configuration files; verify that all deployment configs have been adjusted accordingly.
Dsn string `yaml:"Dsn"`
invoker/runner.go:18
- Using 'return' instead of 'break' ensures the runner thread exits properly upon receiving the stop signal; verify that this change does not bypass any necessary cleanup operations.
return
invoker/job_executor.go:42
- Replacing 'break' with 'return' in the sandbox thread exit path guarantees proper termination; confirm that any resource cleanup is still properly handled.
return
invoker/job.go:33
- Removing the job from ActiveJobs at this point avoids duplicate deletion; ensure that the mutex properly guards all accesses to ActiveJobs.
delete(i.ActiveJobs, j.ID)
invoker/handler.go:23
- [nitpick] The error message now uses the singular form 'invoker job', which improves clarity; please verify that this matches the intended API messaging.
connector.RespErr(c, http.StatusBadRequest, "Can not parse invoker job, error: %s", err.Error())
common/connectors/invokerconn/structs.go:10
- Starting JobType at iota+1 prevents potential zero-value ambiguity; check that this change is compatible with any logic relying on a zero default value.
CompileJob JobType = iota + 1
master/submit.go
Outdated
| // @Failure 400 {object} string | ||
| // @Failure 404 {object} string | ||
| // @Failure 500 {object} string | ||
| // @Router /client/submit [post] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А ты не через swagger тестировал? Я тут забыл поменять путь на /master/submit
| "strconv" | ||
| "testing_system/lib/logger" | ||
|
|
||
| swaggo "github.com/swaggo/files" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ну да, видимо не пользовался) Тут нужен импорт пакета, который сгенерил swagger. Что-то типо import _ "testing_system/swag". После этого ручка должна появиться в /swagger/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Всм, а как это поможет то запросы делать? У нас же просто свагер генерит документацию как бы
| if !ok { | ||
| break | ||
| } | ||
| stackTrace += fmt.Sprintf("%s:%d\n", file, line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно просто stackTrace := string(debug.Stack()). Ну или ещё имя функции добавлять, через runtime.FuncForPC(pc).Name(), чтобы вообще красиво было.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А я тут как раз хочу более короткий лог сделать, а то мне всегда сильно проще именно на строки смотреть, а не на фул стектрейс с именами функций. Все равно если чето не так пошло, то ты сидишь и весь код читаешь, и просто имена функций ничего не дадут
Хм, все таки uuidv6 такое себе, v7 лучше работает |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces various bug fixes and improvements related to job handling, error reporting, and configuration changes in the testing system. Key changes include:
- Switching cancellation contexts and control flow in invoker threads and sandbox runners to better handle thread shutdown.
- Adjustments to job result reporting (from Panic to Error) and removing duplicate job removals.
- Updates to database model JSON/YAML tags, support for in-memory databases, connector error handling improvements, and removal of unused verdict constants.
Reviewed Changes
Copilot reviewed 59 out of 61 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| invoker/runner.go | Changed thread cancellation from i.TS.StopCtx to i.RunnerCtx and improved shutdown flow. |
| invoker/job_executor.go | Added sandbox directory creation and refined sandbox runner shutdown, ensuring proper job deletion. |
| invoker/job.go | Moved ActiveJobs deletion before sending job results and replaced panic calls with error logging. |
| invoker/invoker.go | Updated invoker address to include protocol, added RunnerCtx/RunnerStop, and switched uuid version. |
| invoker/handler.go | Fixed an error message typo in job parsing. |
| common/testing_system.go | Refactored router initialization, server start/stop, and process panic handling with stack tracing. |
| common/db/models/submission.go & common/db/models/problem.go | Changed JSON/YAML field names to uppercase for consistency. |
| common/db/db.go | Introduced support for in-memory SQLite and improved DB connectivity errors. |
| common/constants/verdict/verdict.go | Removed unsupported verdict constants (QD and CL). |
| common/connectors/* | Enhanced connector functions to utilize context and better error parsing. |
| common/config/db.go | Updated DSN configuration key and added an InMemory flag for testing. |
Files not reviewed (2)
- go.mod: Language not supported
- invoker/testdata/compiler/scripts/cpp.sh.tmpl: Language not supported
Comments suppressed due to low confidence (3)
common/constants/verdict/verdict.go:22
- The removal of QD and CL verdict constants might lead to issues if other parts of the system depend on these verdict types. Verify that all consumers of verdict constants have been updated accordingly.
- QD Verdict = "QD" // Queued
common/connectors/invokerconn/structs.go:10
- Changing the enumeration for CompileJob from a 0-based index to starting at iota+1 may disrupt any logic that assumes a 0 value for CompileJob. Confirm that all dependent components are adjusted to handle the updated constant values.
CompileJob JobType = iota + 1
invoker/invoker.go:85
- Prepending 'http://' to the invoker address changes its format and may affect components expecting a host:port string. Ensure that all consumers of i.Address are adjusted to handle the new URL format.
i.Address = "http://" + host + ":" + strconv.Itoa(i.TS.Config.Port)
Пока из багов я вижу что иногда бывает что джоба пропадает из статуса инвокера а потом возрождается. Надо это поисследовать, возможно с ID статуса какие-то баги