-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Chunk taskArns into groups of 100 #1209
Conversation
provider/ecs.go
Outdated
sliceEnd = len(taskArns) | ||
} | ||
split_taskreq = append(splitTaskReq, taskArns[i:sliceEnd]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the splitting logic into a small function, e.g.,
func chunkedTaskArns(taskArns []*string) [][]*string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests on the newly created function would also be nice to cover the representative cases (e.g., 0, 1, 99, 100, 101, 200, 400).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good thinking! I'll catch that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, my git-fu is weak and I managed to commit a broken patch :( fixes soon.
@owen shouldn't we rebase this against |
@owen how's your git-fu going today? ;) also, do you want to rebase against 1.2 as @emilevauge suggested? The bugfix seems fairly important for ECS users. |
it's woeful! I'm king of the force push I guess. I've:
and will be force pushing again as soon as I ensure it passes a Should I keep a PR against master with this code, or do you all typically pull v1.2 fixes back in? not sure how that typically works for this project. |
@owen we typically sync back the v1.2 branch with master regularly (usually on release of a release candidate / final version), so there should be no need to maintain a separate branch against master. |
provider/ecs.go
Outdated
Tasks: taskArns, | ||
Cluster: &provider.Cluster, | ||
}) | ||
var splitTaskReq [][]*string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple comment - as an outsider interrested on this fix:
Is adding a comment to explain why we split the content & do multiple calls will add any value for future maintenance ?
Without knowing the API limitation - or not having the PR context in mind - the 100
split value can be interpreted as coming from nowhere / seems a bit magical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on that one; plus, moving the logic into a dedicated function with an informative name (as suggested previously :-) )
I think @owen is already working hard on this one. :-)
A test case failure I observed last night made me realize that pointing Traefik to an empty cluster would result in bad AWS API calls being made. I've gone ahead and picked up that fix by adding an early return after the first call to ListTasks. This now works for my empty cluster, and my ~hundred instance cluster. Tests pass, |
@timoreimann / @emilevauge - I'm wondering if I broke your CI environment with my, er, aggressive Git usage. Let me know if I can help :( |
@owen yeah, it seems Travis needs a holiday... Could you push force again? |
Looks like the CI is building now. I added a few more tests to wake up the CI pipeline. Apologies... I actually just learned that "rebase master -> v1.2" is as easy as a click at the top of the pull request. That would've skipped at least one force push on my end, heh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few very small suggestions left; after that, we should be good. :-)
provider/ecs.go
Outdated
Tasks: taskArns, | ||
Cluster: &provider.Cluster, | ||
}) | ||
if len(taskArns) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a short comment here on why we return early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
provider/ecs_test.go
Outdated
} | ||
|
||
if !reflect.DeepEqual(outCount, c.expectedLengths) { | ||
t.Fatalf("Chunking %d elements, expected %#v, got %#v (case %d)", c.count, c.expectedLengths, outCount, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do t.Errorf
here: If one case fails, we still want to see the result of the other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think we can drop the index variable i
here. c.count
already suffices to identify the failing case unambiguously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why I Fatalf
'ed there rather than Errorf
'ed - fixed :)
@owen you haven't go fmt'ed your latest commit -- please run Could you also squash your commits? After that and another successful CI run, I think we are good to go/merge. :-) |
If the ECS cluster has > 100 tasks, passing them to ecs.DescribeTasksRequest() will result in the AWS API returning errors. This patch breaks them into chunks of at most 100, and calls DescribeTasks for each chunk. We also return early in case ListTasks returns no values; this prevents DescribeTasks from throwing HTTP errors.
I fear Travis may not be happy with me again:
|
gopkg.in is flaking again. Retriggered the build. |
Grrr, and now the integration tests are flaking. :( Retrying HARDER... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @owen 👍
If the ECS cluster has > 100 tasks, passing them to
ecs.DescribeTasksRequest() will result in the AWS API returning
errors.
This patch breaks them into chunks of at most 100, and calls
DescribeTasks for each chunk.