Skip to content
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

Feature Request: Add RunspaceID handling to Start-RSJob for better throttling support #75

Closed
MVKozlov opened this issue Jun 10, 2016 · 17 comments
Assignees
Milestone

Comments

@MVKozlov
Copy link
Contributor

for now
1..25 | Start-RSJob -throttle 5 can work as expected
and
1..25 | foreach { $_ | Start-RSJob -throttle 5 } is not
because latest creates it own RunspaceID for each new job

Will be better if Start-RSJob can have a possibility to add new job to already existing RunSpacePool
may be with -AddToExistingRunspace switch or -RunspacePoolID parameter

that will need existing pool checking instead of $RunspacePoolID = [guid]::NewGuid().ToString() [....]
i.e. invoke new RunspacePool code only if selected with -RunspaceID parameter is not found

@proxb proxb self-assigned this Jun 10, 2016
@proxb
Copy link
Owner

proxb commented Jun 10, 2016

I think I was looking at this at one point in my initial module build but ended up shelving it for one reason or another. This is something that I will definitely revisit and see about adding in a future release.

@MVKozlov
Copy link
Contributor Author

After some thinking, I realize that this feature will look better if it will be implemented as -LinkThreadPoolToBatchName (or may be this behaviour should be default ?)
Because thread throttling as usual needed for similar tasks that already grouped by -Batch name and threadpool management out of user control.

@proxb
Copy link
Owner

proxb commented Jun 27, 2016

That makes sense to me. I'll explore this probably in the coming weeks once I get a couple of other things knocked that I am working on.

@proxb proxb added this to the 1.6.0.0 milestone Jun 27, 2016
@proxb
Copy link
Owner

proxb commented Jun 28, 2016

I think what I will do is automatically generate a Batch name (as a guid or something like that) if one is not given and link that to the runspacepool and if the user decides to use a batch name, then that will be used to create the runspacepoolID as well. I'll need to update the DLL file to make the runspacepoolid a string as opposed to a guid.

@MVKozlov
Copy link
Contributor Author

MVKozlov commented Jul 3, 2016

Good - lets runspaceID will be equal to batch name.

then that will be used to create the runspacepoolID

Not just create, but add new job to existing runspace with the same name/id

@proxb
Copy link
Owner

proxb commented Jul 4, 2016

Not just create, but add new job to existing runspace with the same name/id

Assuming the existing runspacepool hasn't been disposed of by that time, in which case a new runspacepool would be created. This reminds me that I should look at adjusting the current time before a runspacepool is disposed of. Currently it is after a minute of inactivity but I may bump it to 5 minutes with this addition.

@proxb
Copy link
Owner

proxb commented Aug 5, 2016

Made some progress on this with the linking of Batch to RunspacePoolID and most of my testing showed that this was working great. The only thing holding me back now is providing some extra code for those who use $_ within the script under a ForEach (or similiar) loop which currently only works when piping directly to the function. I'm probably going to have to some sort of scriptblock inspection to see if the variable exists outside of any other loop and treat it as though it came from the pipeline.

1..5|ForEach {
Start-RSJob -ScriptBlock {$_}
}

@proxb
Copy link
Owner

proxb commented Aug 5, 2016

I may have been overthinking my last comment example. The example you showed works great and I am just leave it at that although I did find a way to detect if the function is part of a ForEach command loop... I might test that just a bit and if it doesn't work like I want it to then I will scrap it and publish the current update that I have this weekend.

proxb added a commit that referenced this issue Aug 12, 2016
@proxb
Copy link
Owner

proxb commented Aug 12, 2016

@MVKozlov
If you get a chance, try out the latest version update and let me know if it meets what you are looking for.

@MVKozlov
Copy link
Contributor Author

MVKozlov commented Aug 15, 2016

Thanks :)
it works exactly as needed

bonus:
Test-RSJob.ps1 https://gist.github.com/MVKozlov/a093877010c213564dd4f638724ff9d4
Test-RSJob.Tests.ps1 https://gist.github.com/MVKozlov/f3754975db518a9b096b32d8503e588b

@proxb
Copy link
Owner

proxb commented Aug 16, 2016

Cool! I'll get that added to my existing Pester tests.

@proxb
Copy link
Owner

proxb commented Aug 18, 2016

I'm leaving this open until I can add the Pester test.

@proxb proxb added the Pester label Aug 18, 2016
@proxb proxb modified the milestones: 1.7.1.1, 1.6.0.0, 1.7.2.1 Aug 27, 2016
@proxb
Copy link
Owner

proxb commented Aug 30, 2016

Pester test added to existing tests

@proxb proxb closed this as completed Aug 30, 2016
proxb added a commit that referenced this issue Sep 29, 2016
#101 fixed and #108 fixed and #75 change rolled back
@proxb proxb reopened this Sep 29, 2016
@proxb
Copy link
Owner

proxb commented Sep 29, 2016

Reopening as this brought on a breaking change (#108). I am still looking to include this functionality but need to spend more time researching and testing to ensure it doesn't break core functionality of the module.

@proxb proxb removed the Pester label Sep 29, 2016
@MVKozlov
Copy link
Contributor Author

I do not have Issues as in #108 (leave comment about it)
Can help with any further testing (excl. weekends :)

MVKozlov pushed a commit to MVKozlov/PoshRSJob that referenced this issue Oct 3, 2016
@AspenForester
Copy link

@MVKozlov is your Pester test from August 15 supposed to pass on the "Full Pipe input", and fail on the "OneByOne Pipe input"?

@MVKozlov
Copy link
Contributor Author

MVKozlov commented Oct 28, 2016

@AspenForester , my Pester test works as intendent (all pass) on post-1.6.1.0 and pre-1.7.2.9 or on my 1.7.2,9 fork - #109

@proxb proxb modified the milestones: 1.7.3.0, 1.7.2.1, 1.7.3.3 Nov 10, 2016
proxb added a commit that referenced this issue Nov 13, 2016
Compromise between issues #75 and #108
proxb added a commit that referenced this issue Nov 13, 2016
@proxb proxb closed this as completed Nov 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants