feat(api): add asynchronous benchmark run creation - #25
Conversation
Signed-off-by: amh1k <abdulmoizx97@gmail.com>
Signed-off-by: amh1k <abdulmoizx97@gmail.com>
60c2d8e to
fa4cbc1
Compare
| result, err := a.runner.Run(ctx, connection, options) | ||
| status, message := RunStatusSucceeded, "" | ||
| if err != nil { | ||
| status, message = RunStatusFailed, "benchmark execution failed" |
There was a problem hiding this comment.
this is not good. if job fails you just right down this error and nothing is logged. it is impossible to debug what happened. need a bit more love to logging.
There was a problem hiding this comment.
Makes sense have implemented basic logging in the new commit
| if err := a.store.Complete(id, status, result, message); err != nil { | ||
| // A lost run record cannot be repaired here, but the execution goroutine | ||
| // must still release its concurrency slot. | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The slot is already released by the defer, so the comment above it is misleading. Replace it with a log line.
| for _, credentials := range []*everest.Credentials{ | ||
| {Host: "postgres.example.com", Port: "5432", Username: "user", Password: "secret", Provider: "provider-mongodb", Type: "mongodb"}, | ||
| {Host: "postgres.example.com", Port: "5432", Username: "user", URI: "postgres://user:secret@host/db", Provider: "provider-cloudnative-pg", Type: "postgresql"}, | ||
| } { |
There was a problem hiding this comment.
I think this is ok for MVP, but hardcoding provider names is a no go for a long run
| type API struct { | ||
| getCredentials CredentialLookup | ||
| runner BenchmarkRunner | ||
| store *RunStore | ||
| lifecycleCtx context.Context | ||
| runSlots chan struct{} | ||
| } |
There was a problem hiding this comment.
Nothing waits for the background runs. Cancelling lifecycleCtx does start cleanup, since the coordinator uses WithoutCancel plus a timeout. But main can return before the Job and the Secret holding the DB password are deleted. Please track the runs and expose a Shutdown method so the wiring PR can wait for them.
A bare WaitGroup isn't enough: calling Add while Wait is in progress is a race. The Add and the "closing" flag need to share one mutex.
There was a problem hiding this comment.
implemented a fix for this in the commit
| token, err := extractBearerToken(r) | ||
| if err != nil { | ||
| w.Header().Set("WWW-Authenticate", "Bearer") | ||
| http.Error(w, err.Error(), http.StatusUnauthorized) |
There was a problem hiding this comment.
http.Error returns text/plain, but the existing main.go handlers return JSON {"error": "..."} through writeJSON. The UI shouldn't have to handle both. Please add one helper in the api package and use it everywhere.
There was a problem hiding this comment.
created a new helper function for writing errors
Signed-off-by: amh1k <abdulmoizx97@gmail.com>
2 participants
Summary
Adds
POST /api/runsto create PostgreSQL benchmark runs asynchronously against OpenEverest-managed databases.Changes
202 Acceptedwith its ID.429when capacity is full.Testing
go test ./...go test -race ./internal/apiTests cover request validation, credential lookup, error handling, and asynchronous execution.
Remaining work
Backend startup and route wiring in
main.goare still pending. Status/result retrieval and bounded retention are covered by the second subissue.