Consumer groups #89
Consumer groups #89
Conversation
…ome of the code in consumergroup to accomodate conditional expressions. Moved the LeaseUpdate from consumergroup to ddb.go - it is only needed by ddb.go, and it is ddb.go specific. Added some comments.
| // shards on a regular cadence. | ||
| func (b *broker) start(ctx context.Context) { | ||
| b.findNewShards() | ||
| func (b *broker) Start(ctx context.Context, shardc chan string) { |
There was a problem hiding this comment.
we could potentially adjust this func signature to have it return chan string
func (b *broker) Start(ctx context.Context) chan string {
consumergroup.go
Outdated
| OwnerID string | ||
| done chan struct{} | ||
| currentLeases map[string]*Lease //Initially, this will only be one | ||
| Mutex *sync.Mutex |
There was a problem hiding this comment.
can we use a Mutext Hat here:
https://dmitri.shuralyov.com/idiomatic-go#mutex-hat
Also I think the exported vs non-exported should be grouped together in alphabetical order
There was a problem hiding this comment.
re-arranged. This is good idea for all go. I wish the reformatters in IDEs would do this.
consumergroup.go
Outdated
| for { | ||
| select { | ||
| case <-tick.C: | ||
| if len(cgc.currentLeases) == 0 { // only do anything if there are no current leases |
There was a problem hiding this comment.
can we reduce indentation and use a guard clause?
if len(cgc.currentLeases) > 0 {
continue
}
// ...
consumergroup.go
Outdated
| } | ||
|
|
||
| lease := cgc.CreateOrGetExpiredLease(currentLeases, previousLeases) | ||
| if lease != nil && lease.LeaseKey != "" { |
There was a problem hiding this comment.
can we invert this conditional too? ideally the end of this loop will be the core functionality and not indented
consumergroup/ddb.go
Outdated
| "github.com/aws/aws-sdk-go/service/dynamodb/dynamodbattribute" | ||
| "github.com/aws/aws-sdk-go/service/dynamodb/expression" | ||
|
|
||
| consumer "github.com/harlow/kinesis-consumer" |
There was a problem hiding this comment.
feels like we might be in danger of cyclical import. I've tried to stick to general rule of thump that sub packages don't know about directory levels above them.
There was a problem hiding this comment.
I think we might be fine once we reorganize the packages, but if not, let's revisit this guy. The only thing it is using right now is the Lease struct.
There was a problem hiding this comment.
I moved Lease to storage - in a new file called lease.go - all storage options should be able to use this. Let me know if that works.
separated out lease and moved to storage, created storage.go.
|
hi @keperry I'm gonna close this PR for now (I can't approve PRs I open). that way you can rebase, and submit a PR from your account . |
No description provided.