Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

mt-update-ttl multithreaded + more #843

Merged
merged 7 commits into from
Feb 22, 2018
Merged

mt-update-ttl multithreaded + more #843

merged 7 commits into from
Feb 22, 2018

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Feb 5, 2018

@shanson7 did some very nice multi-threading work, resulting in a big speedup. i added a bit of polish on top

@Dieterbe Dieterbe changed the title Mt update ttl fixes mt-update-ttl multithreaded + more Feb 5, 2018
return 0.5 - float64(token)/float64(2*minToken)
}
return 0.5 + float64(token)/float64(2*maxToken)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be simplified to ((token/maxToken)+1)/2 in both cases.

"time"

"github.com/gocql/gocql"
hostpool "github.com/hailocab/go-hostpool"
"github.com/raintank/dur"
)

const maxToken = 9223372036854775807
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a nice way to calculate the completeness, but it took me quite a while to understand how it works, so a comment explaining what this number is and why it can be used to get the completeness based on the return of the token() function would be helpful

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also could use math.MaxInt64


var wg sync.WaitGroup
for i := 0; i < *numThreads; i++ {
wg.Add(1)
Copy link
Contributor

@replay replay Feb 6, 2018

Choose a reason for hiding this comment

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

this could be moved before the loop and simply wg.Add(*numThreads)

return ((float64(token) / float64(maxToken)) + 1) / 2
}

func worker(id int, jobs <-chan string, wg *sync.WaitGroup, session *gocql.Session, startTime, endTime, ttl int, tableIn, tableOut string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the id used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't, now is

fmt.Println("processed", rownum, "rows")

wg.Wait()
log.Printf("DONE. Processed %d keys, %d rows", doneKeys, doneRows)
Copy link
Contributor

@replay replay Feb 6, 2018

Choose a reason for hiding this comment

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

might want to add

if err != nil {
    os.Exit(2)
}

after the wg.Wait(), just to make sure that something else calling this tool never falsely assumes everything worked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

that's not important, but why not just

err := keyItr.Close()
wg.Wait()
if err != nil {
    fmt.Fprintf(os.Stderr, "ERROR: failed querying %s: %q. processed %d keys, %d rows", tableIn, err, doneKeys, doneRows)
    os.Exit(2)
}
log.Printf("DONE.  Processed %d keys, %d rows", doneKeys, doneRows)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, that would save a line. otoh this way has the err handling right after the operation, which is nice for readability. also you can see the error without having to wait for all cassandra operations in the workers to complete.

@Dieterbe
Copy link
Contributor Author

all comments adressed.

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

LGTM with one more comment

@Dieterbe Dieterbe merged commit 68e8b03 into master Feb 22, 2018
@Dieterbe Dieterbe deleted the mt-update-ttl-fixes branch September 18, 2018 08:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants