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

Exposing Create and CreateAsync in collection that takes variable number of Creatables #1015

Merged
merged 7 commits into from
Aug 10, 2016
Merged

Exposing Create and CreateAsync in collection that takes variable number of Creatables #1015

merged 7 commits into from
Aug 10, 2016

Conversation

anuchandy
Copy link
Member

No description provided.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@azuresdkci
Copy link
Contributor

Runtime changes detected. pull request created. CI running: Build Status

@martinsawicki
Copy link

I like it, but I'd like Jianghao to take a look as well.

@jianghaolu
Copy link
Contributor

jianghaolu commented Aug 9, 2016

Looking good! It's cool that everyone gets the functionality for free.
Just one question, is there a scenario where something creatable is not batch creatable? In other words, can we merge SupportsBatchCreating into SupportsCreating?

@anuchandy
Copy link
Member Author

Thanks jianghao, yes there is one scenario where a collection that exposes define(string name) cannot expose Create(Creatable<T>... creatables), the dependency graph requires all creatable resources are of type Resource. The Deployment is not a Resource. This is why we bound generic parameter of SupportsBatchCreation to Resource

public interface SupportsBatchCreation<ResourceT extends Resource> {
...
}

@jianghaolu
Copy link
Contributor

@martinsawicki
Can you share the user scenario behind this feature? I was trying to understand the goal of this PR and that can't really go far without knowing the user scenario. Thanks!

@jianghaolu
Copy link
Contributor

Also, one potential problem is for the async method. For sync methods, we have no problem calling:

List<VirtualMachine> vms = virtualMachines.create(vm1, vm2);

For async, this doesn't compile:

ServiceCall<List<VirtualMachine>> vmsFuture = virtualMachines.createAsync(vm1, vm2, null); // BTW, let's consistently keep callback as the last param since it's optional
/* do a bunch of stuff here */
List<VirtualMachine> vms = vmsFuture.get();

@anuchandy
Copy link
Member Author

anuchandy commented Aug 9, 2016

The async issue seems more general [i.e. storing the return value of async with return type ServiceCall<Derived> to a variable of type ServiceCall<Base> ]. For example if we use SDK like below, it compiles.

  ServiceCall<List<VirtualNetworkInner>> list = this.innerCollection.listAllAsync(null);

but not the below code, though List inherits from Collection

  ServiceCall<Collection<VirtualNetworkInner>> list1 = this.innerCollection.listAllAsync(null);

[Below code compiles]

  ServiceCall<VirtualNetworkInner> v = this.innerCollection.getAsync(null, null, null);

[below one not ]

  ServiceCall<com.microsoft.azure.Resource> v1 = this.innerCollection.getAsync(null, null, null);

VirtualNetworkInner inherits com.microsoft.azure.Resource

@martinsawicki
Copy link

@jianghaolu as far as the scenario question goes, the main thing I see with this is that I don't have to worry about asynch programming patterns to be able to take advantage of parallelism in the creation of independent resources. Our own automation test cases are a simple case of that --- to test aspects of an LB, a bunch of resources should already exist: VMs, PIPs, VNets. SO the test should pre-create them, but doing it all synchronously is very slow.

Exposing this on a collection of specific resources is a step toward the even more flexible scenario I think we could fairly easily enable as well -- of accepting a list of arbitrary Creatables from the user (via e.g. Azure#create(...)) and have the SDK parallelize it as needed. Since we already have that logic done internally, this should also be relatively simple to expose to users.

Ultimately, I can easily envision users pursuing a pattern (in such more complex multi-resource situations) of not calling individual <Resource>.create() at all, but only dealing with Creatables and passing them all to a single .create() at the end (or .createAsynch()).

This could perhaps marry the best of both worlds - the intuitiveness, familiarity and debuggability of the imperative synchronous programming model with the performance of the declarative and asynchronous ARM template model.
(It also has more of that Promise-like feel to it, where asynch operations are still ordered based on their dependency trees and failures can be handled in one place)

Just my thoughts on that.

@jianghaolu jianghaolu merged commit 4136dd4 into Azure:master Aug 10, 2016
jianghaolu added a commit to jianghaolu/azure-sdk-for-java that referenced this pull request Aug 27, 2016
6c35000 Fix tests for fluent code
a1cd0f1 Paging async works
148e463 Generate correct paging code
57cbe08 LRO tests pass
bae30e0 Regenerate fluent
0882500 Merge commit '6eb93f97e4e2faab3b8e56090fc697c32dd2f410' into rx
2e44bb0 User observable for paging
0fffe57 Merge 9925f2c into da82c36
b7c1094 Remove unused imports
b19b716 LRO now uses Rx
75a68c3 Generate Observable based clients in generic generator
0a7e455 Create VM works
5bb56b9 Fixing the javadoc error and formatting errors for key vault
16c5acd Observable based async works for NSG
c039c6e Merge pull request Azure#1015 from anuchandy/resources-create
44a1519 Merge branch 'master' of github.com:Azure/azure-sdk-for-java into resources-create
652d622 Exposing Create and CreateAsync in collection that takes variable number of Creatables

git-subtree-dir: runtimes
git-subtree-split: 6c35000
@anuchandy anuchandy deleted the resources-create branch February 8, 2017 23:46
jianghaolu added a commit to jianghaolu/azure-sdk-for-java that referenced this pull request Feb 27, 2017
Exposing Create and CreateAsync in collection that takes variable number of Creatables
jianghaolu added a commit to jianghaolu/azure-sdk-for-java that referenced this pull request Feb 27, 2017
6c35000 Fix tests for fluent code
a1cd0f1 Paging async works
148e463 Generate correct paging code
57cbe08 LRO tests pass
bae30e0 Regenerate fluent
0882500 Merge commit '6eb93f97e4e2faab3b8e56090fc697c32dd2f410' into rx
2e44bb0 User observable for paging
0fffe57 Merge 9925f2c into da82c36
b7c1094 Remove unused imports
b19b716 LRO now uses Rx
75a68c3 Generate Observable based clients in generic generator
0a7e455 Create VM works
5bb56b9 Fixing the javadoc error and formatting errors for key vault
16c5acd Observable based async works for NSG
c039c6e Merge pull request Azure#1015 from anuchandy/resources-create
44a1519 Merge branch 'master' of github.com:Azure/azure-sdk-for-java into resources-create
652d622 Exposing Create and CreateAsync in collection that takes variable number of Creatables

git-subtree-dir: runtimes
git-subtree-split: 6c35000
jianghaolu added a commit to jianghaolu/azure-sdk-for-java that referenced this pull request Feb 27, 2017
Exposing Create and CreateAsync in collection that takes variable number of Creatables
sima-zhu pushed a commit to sima-zhu/azure-sdk-for-java that referenced this pull request Mar 21, 2019
Exposing Create and CreateAsync in collection that takes variable number of Creatables
sima-zhu pushed a commit to sima-zhu/azure-sdk-for-java that referenced this pull request Mar 21, 2019
[Automatic PR] SDK changes from pull request Azure#1015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants