Skip to content
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

Implementation of batch processing #48

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

tgvashworth
Copy link
Collaborator

@tgvashworth tgvashworth commented Oct 3, 2022

This commit does quite a bit:

- Introduces Makefile as the commands are getting long
- Adds installation of ImageMagick to the Dockerfile
- Adds imagick bindings
- Instatiates the bindings in main.go to check they work
@tgvashworth tgvashworth force-pushed the impl/batch-processing-2 branch from ba7c147 to 5a0d78f Compare October 3, 2022 09:56
@tgvashworth tgvashworth marked this pull request as ready for review October 3, 2022 19:13
@tgvashworth tgvashworth self-assigned this Oct 3, 2022
@tgvashworth tgvashworth modified the milestone: Sprint 1 Complete Oct 3, 2022
Copy link
Member

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks like a good shape :) I'm assuming it'll be come more goroutine-based at some point? I definitely like the breaking up of IO-bound and CPU-bound work into separate goroutine pools :)

batch-processing/Dockerfile Outdated Show resolved Hide resolved
batch-processing/IMPLEMENTATION.md Show resolved Hide resolved
batch-processing/IMPLEMENTATION.md Show resolved Hide resolved
batch-processing/main.go Outdated Show resolved Hide resolved
batch-processing/main.go Outdated Show resolved Hide resolved
batch-processing/main.go Outdated Show resolved Hide resolved
defer res.Body.Close()

// Ensure we got success from the server
if res.StatusCode != http.StatusOK {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an extension problem in the README, I'd add trying to get them thinking about pause/resume, so that already-done work can be skipped if we end up re-running (i.e. if the processed failed, we somehow have tracked what we've already processed, so we can just pick up the not-yet-done work). But not for the first cut!

However, for the example impl I'd probably add a comment here contrasting the pros of eagerly failing vs log and continue

@tgvashworth tgvashworth force-pushed the impl/batch-processing-2 branch from 72910a0 to 022f41a Compare October 4, 2022 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Develop simple batch-processing project
2 participants