Conversation
hojulian
left a comment
hojulian
left a comment
There was a problem hiding this comment.
I like where this is going, looks good for most of it. Missing the crucial StartTask part but that's it. Good work!
| "github.com/Capstone-auto-grader/grader-api-v2/internal/grader-task" | ||
| "github.com/Capstone-auto-grader/grader-api-v2/internal/graderd" | ||
| "github.com/Capstone-auto-grader/grader-api-v2/internal/sync-map" | ||
| "github.com/docker/docker/api/types" | ||
| "github.com/docker/docker/client" | ||
| "io/ioutil" | ||
| "log" | ||
|
|
||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
try to organize packages as groups: https://github.com/golang/go/wiki/CodeReviewComments#imports
There was a problem hiding this comment.
^ same for other files as well
| "github.com/Capstone-auto-grader/grader-api-v2/internal/docker-client" | ||
| sync_map "github.com/Capstone-auto-grader/grader-api-v2/internal/sync-map" |
There was a problem hiding this comment.
should use single word names: https://golang.org/doc/effective_go.html#package-names
| } | ||
| grpcServer := grpc.NewServer(grpc.Creds(serverCert)) | ||
| graderService := graderd.NewGraderService(graderd.NewDockerClient(*dockerAddr, *dockerVersion), graderd.NewPGDatabase(*databaseAddr), *webAddr) | ||
| graderService := graderd.NewGraderService(docker_client.NewDockerClient(*dockerAddr, *dockerVersion, sync_map.NewSyncMap(), 2), *webAddr) |
There was a problem hiding this comment.
not sure about creating and passing the sync_map in the constructor of docker_client, any reason why we don't just create it inside the constructor?
| _ = tasks.UpdateStatus(taskId, grader_task.StatusStarted, true) | ||
| _ = client.StartTask(context.Background(), taskId) | ||
| _,_ = client.TaskOutput(context.Background(), taskId) |
There was a problem hiding this comment.
I think we can make another channel here for propagating errors out.
There was a problem hiding this comment.
I was thinking a channel for all responses and errors, so another worker can deal with them
| for _, t := range tasks { | ||
| if t.ContainerID == c.ID { | ||
| status := graderd.ParseContainerState(c.State) | ||
| if status == grader_task.StatusStarted { |
There was a problem hiding this comment.
should we show the tasks that stopped unexpectedly as well, just a thought.
| "sync" | ||
| "github.com/Capstone-auto-grader/grader-api-v2/internal/grader-task" | ||
| ) | ||
| // A synchronized map to store Tasks |
There was a problem hiding this comment.
comment should start with "SyncMap is ..."/"SyncMap ... ": https://golang.org/doc/effective_go.html#commentary
| type DockerClient struct { | ||
| cli *client.Client | ||
| mp *sync_map.SyncMap | ||
| queue chan<- string |
There was a problem hiding this comment.
make this two-way, otherwise you won't be able to pass stuff in.
| //} | ||
|
|
||
| func (d *DockerClient) StartTask(ctx context.Context, taskID string) error { | ||
| return nil |
There was a problem hiding this comment.
the important part is missing!!!
There was a problem hiding this comment.
I'm hammering that one out after I write the rest of it!
| defer m.mu.Unlock() | ||
| t := m.mp[taskID] | ||
| if checkStatus && t.Status == grader_task.StatusStarted { | ||
| return fmt.Errorf("task already started") |
There was a problem hiding this comment.
use errors.New(...) instead
| return nil | ||
| } | ||
|
|
||
| func (m *SyncMap) Enumerate() map[string]grader_task.Task { |
There was a problem hiding this comment.
im not very sure about this approach. Can you explain more on why we need to copy the whole map everytime? When the use-case is just readonly?
There was a problem hiding this comment.
I'm worried about a concurrent update to some of the elements of the task (status or containerID) in the middle of reading. Thinking about it more, I don't think it would be the worst thing in the world to have an intermediate state read, what do you think @xumr0x ?
2 participants
No description provided.