2
\$\begingroup\$

I created the following one off script and it did the task quite well (1000s of requests / second with low CPU usage - outperformed the previous python script in every aspect). But I am wondering if there are more "gophonic" ways to accomplish this in Golang? (the word for pythonic in Go? :p ) The code was gofmt'd and golint'd.

The result set is variable but in the range between 100k and 20M rows (as such so many copy requests to AWS has to be done).

After some helpful comments I already edited the code once to rename the concurrency flag to reflect the intend of the current (original) implementation better (numberChannels -> numberGoRoutinesForCopy).

My main concern for this review is to have optimal efficiency with large number of concurrent invocations of copyKey independent of where the data came from, resp. what exactly the copying does.

import (
    "flag"
    "log"
    "os"

    "database/sql"
    _ "github.com/go-sql-driver/mysql"

    "github.com/aws/aws-sdk-go/aws"
    "github.com/aws/aws-sdk-go/aws/session"
    "github.com/aws/aws-sdk-go/service/s3"
)

var (
    from = flag.String(
        "from", "2011-01-01 00:00:00",
        "From Date.",
    )
    to = flag.String(
        "to", "2011-12-31 23:59:59",
        "To Date.",
    )
    region = flag.String(
        "aws.region", "us-east-1",
        "AWS region.",
    )
    numberGoRoutinesForCopy = flag.Int(
       "concurrency", 250,
       "Concurrent Requests.",
    )

    sizes = [11]string{
        "catalog",
        "catalog-m",
        "cart",
        "product",
        "product-m",
        "gallery",
        "zoom",
        "related",
        "list",
        "feed",
        "progressproduct",
    }
)    

type imageMetaData struct {
    ID    string
    Sku   string
    Image string
    Size  string
}

func main() {
    log.SetOutput(os.Stdout)
    flag.Parse()
    done := make(chan bool)

    sess := session.Must(session.NewSession())

    svc := s3.New(sess, aws.NewConfig().WithRegion(*region))

    ch := make(chan imageMetaData)
    go getData(*from, *to, ch)

    log.Println(*numberGoRoutinesForCopy)
    for i := 0; i < *numberGoRoutinesForCopy; i++ {
        go func(messages <-chan imageMetaData) {
            for data := range messages {
                copyKey(data, svc)
            }
            close(done)
        }(ch)
    }

    <-done
    log.Println("done")
}


func getData(from string, to string, ch chan imageMetaData) {

    var (
        id    string
        sku   string
        image string
    )

    db, err := sql.Open("mysql", "database_conn")
    db.SetMaxIdleConns(0)
    if err != nil {
        // we won't stop on error
        log.Println(err.Error())
    }
    defer db.Close()

    rows, err := db.Query("SELECT some_big_sql_left_out_for_brevity")
    if err != nil {
        // we won't stop on error
        log.Println(err)
    }
    defer rows.Close()

    for rows.Next() {
        err := rows.Scan(&id, &sku, &image)
        if err != nil {
            // we won't stop on error
            log.Println(err)
        }

        for _, size := range sizes {
            ch <- imageMetaData{
                Id:    id,
                Sku:   sku,
                Image: image,
                Size:  size,
            }
        }
    }

    close(ch)
}    

func copyKey(ImageData imageMetaData, svc *s3.S3) {

    var keyName = getKeyName(ImageData)
    params := &s3.CopyObjectInput{
        Bucket:     aws.String("some_bucket"),
        CopySource: aws.String("some_other_bucket/" + keyName),
        Key:        aws.String(keyName),
    }

    _, err := svc.CopyObject(params)

    if err != nil {
        log.Println(err)
    }

    log.Println(keyName + " copied")
}

func getKeyName(ImageData imageMetaData) string {
    runes := []rune(ImageData.Id)
    for i, j := 0, len(runes)-1; i < j; i, j = i+1, j-1 {
        runes[i], runes[j] = runes[j], runes[i]
    }
    return string(runes) + "/" + ImageData.Image + "-" + ImageData.Size + ".jpg"
}
\$\endgroup\$
  • \$\begingroup\$ There's not much concurrency in here at all, and the numberChannels variable looks useless. You have 3 active routines only, one of them is mostly just waiting on done. your code is missing the flag handling and variables, and also the copyKey function (and that function will have a big impact on the need for concurrency depending on what that does). \$\endgroup\$ – rolfl Mar 31 '17 at 15:30
  • \$\begingroup\$ @rolfl I added the rest of the code, in fact I mean to get high throughput - so I start numberChannels of goroutines to process the data (requests to s3) \$\endgroup\$ – Georg Buske Mar 31 '17 at 16:35
  • \$\begingroup\$ Just a little question: why won't you stop on error? Regarding performance, I don't know how exactly channels in Go work, but maybe it would be more efficient to create one channel per copyKey goroutine and alternately send data into these channels in getData. \$\endgroup\$ – kyrill Mar 31 '17 at 17:06
  • 1
    \$\begingroup\$ OK then. Maybe try with more goroutines, or less channels per goroutine (eg. 1000 goroutines with 50 channels, etc). But it's possibly most efficient with just one channel. \$\endgroup\$ – kyrill Mar 31 '17 at 19:24
  • 1
    \$\begingroup\$ How do you know, for sure, that by the time close(ch) is called, the processing routine has done all it needed to do? Use sync.WaitGroup for things like that. Rule of thumb is also: close a channel in the routine that creates it. What you're doing with done is not considered best practices. Just push a value onto a non-buffered channel: reads and writes block in that case \$\endgroup\$ – Elias Van Ootegem Apr 5 '17 at 15:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.