Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

add buffering/queueing function in default Group plugin #332

Merged
merged 4 commits into from
Dec 13, 2016

Conversation

YujiOshima
Copy link
Contributor

Related #331
Add max-parallel flag to set maximum number of parallel provision.
Signed-off-by: Yuji Oshima yuji.oshima0x3fd@gmail.com

@YujiOshima
Copy link
Contributor Author

CI error will fix by #333

@codecov-io
Copy link

codecov-io commented Dec 12, 2016

Current coverage is 69.08% (diff: 65.38%)

Merging #332 into master will increase coverage by 0.03%

@@             master       #332   diff @@
==========================================
  Files            30         30          
  Lines          1551       1569    +18   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1071       1084    +13   
- Misses          373        380     +7   
+ Partials        107        105     -2   

Powered by Codecov. Last update 6a203f3...503f4fb

@@ -242,6 +259,10 @@ func (s *scaler) converge() {

s.scaled.CreateOne(nil)
}()
if s.maxParallelNum > 0 && (i+1)%int(s.maxParallelNum) == 0 {
Copy link
Contributor

@chungers chungers Dec 12, 2016

Choose a reason for hiding this comment

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

How about extract this into a function on the scaler:

func (s *scaler) waitIfReachParallelLimit(current int, batch *sync.WaitGroup) {
   // call wait on the batch if the current index reaches the parallel limit
}

and then call this as s.waitIfReachParallelLimit(i, &grp), and also after line 248 so that we can rate limit the Destroy calls as well?

@@ -13,7 +13,7 @@ func TestScaleUp(t *testing.T) {
defer ctrl.Finish()

scaled := mock_group.NewMockScaled(ctrl)
scaler := NewScalingGroup(scaled, 3, 1*time.Millisecond)
scaler := NewScalingGroup(scaled, 3, 1*time.Millisecond, 0)
Copy link
Contributor

@chungers chungers Dec 12, 2016

Choose a reason for hiding this comment

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

Can we add a new test or test this with maxParallel = 1? The test should pass fine since we are effectively serializing the CreateOne calls.

Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
Signed-off-by: Yuji Oshima <yuji.oshima0x3fd@gmail.com>
@YujiOshima
Copy link
Contributor Author

@chungers Thank you for your review!
I updated following the comments and rebase #329.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "buffering_instance" git@github.com:YujiOshima/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353234656
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@chungers chungers merged commit 3ccd55c into docker-archive:master Dec 13, 2016
chungers pushed a commit to chungers/infrakit that referenced this pull request Sep 30, 2017
* fixes docker-archive#332

Signed-off-by: Nicola Kabar <nicolaka@gmail.com>

* adding DTR to DDC fixes docker-archive#332

Signed-off-by: Nicola Kabar <nicolaka@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants