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

IRedisCache fluent calls that don't necessarily make sense #304

Closed
TimLovellSmith opened this issue May 3, 2018 · 6 comments
Closed

IRedisCache fluent calls that don't necessarily make sense #304

TimLovellSmith opened this issue May 3, 2018 · 6 comments
Assignees
Milestone

Comments

@TimLovellSmith
Copy link
Member

TimLovellSmith commented May 3, 2018

I just noticed it is funny that you can put multiple sku changes in a single 'update' call like this:

caches.GetById(id).Update().WithPremiumSku().WithStandardSku().WithBasicSku().Apply();

It is also funny that you can do 'WithSubnet' and 'WithStaticIP' when you're chosen basic/standard SKU, as we don't allow that!

INetwork nw;
caches.Define("c").WithRegion("r").WithExistingResourceGroup("g").WithBasicSku().WithSubnet(nw, "subnetName");

@hovsepm hovsepm self-assigned this May 3, 2018
@hovsepm
Copy link
Contributor

hovsepm commented May 10, 2018

for caches.GetById(id).Update().WithPremiumSku().WithStandardSku().WithBasicSku().Apply();

  • unfortunately current Fluent Flow does not allow us to make constraints depending on the runtime value of cache instance. This part would be won't fix

Moved 'WithSubnet' and 'WithStaticIP' to premium Cache definition only in pr Azure/azure-libraries-for-java#445. Thanks for reporting.

@hovsepm hovsepm added this to the v1.11 milestone May 10, 2018
@TimLovellSmith
Copy link
Member Author

TimLovellSmith commented May 11, 2018

@hovsepm
Actually my point was that a reader can see that its nonsense without knowing a runtime value, all you need to know is that it should be impossible to assign multiple skus to a single cache...

A brainstorm of how the compiler could be taught this, in terms of the types involved (but perhaps unrealistic):

interface IRedisCacheDefinitionWithoutSkuSpecified : IRedisCacheWithSkuSpecified
{
     IRedisCacheWithNonPremiumSkuSpecified WithBasicSku();
     IRedisCacheWithNonPremiumSkuSpecified WithStandardSku();
     IRedisCacheWithPremiumSkuSpecified WithPremiumSku();
}

interface IRedisCacheDefinitionWithPremiumSkuSpecified : IRedisCacheWithSkuSpecified
{
    IRedisCacheWithPremiumSkuSpecifiedAndWithSubnetSpecified WithSubnet(); //etc
}

interface IRedisCacheDefinitionWithBasicSkuSpecified: IRedisCacheWithSkuSpecified
{}

@hovsepm
Copy link
Contributor

hovsepm commented May 14, 2018

@TimLovellSmith The issue is in Update stage which (by design) should allow partial modifications of the Redis resource. That is why each transition is optional. In other words each method call in the Update flow returns to the same Updatable state. There does not seems to be a good Update Flow that will guide modifications for a Redis resource (or any other resource). What if developers does not want to change Sku at all? Why should they do something like this:

caches.GetById(id).Update().NoSkuChange().WithThis().WithThat()... etc

I believe in create (aka definition) flow developers are not able to switch between SKUs.

@TimLovellSmith
Copy link
Member Author

TimLovellSmith commented May 14, 2018

@hovsepm For update, we can take shardCount as an example since its one of the premium SKU settings you can update.

I don't mind that you can do redisCache.Update().WithShardCount(2), in fact I agree that when you don't know the actual SKU, it is better to be permissive.

It was just the doomed-to-fail combinations which I felt I should try to object to: .WithBasicSku().WithShardCount(2), WithBasicSku().WithStandardSku() etc.

@hovsepm
Copy link
Contributor

hovsepm commented May 15, 2018

@TimLovellSmith I think we should consider separating Redis and RedisPremium types in the Fluent layers. In that case we can have better functionality and isolation of features but that work will be a major breaking change for customers and most probably will be done during 2.0 transition of Fluent SDK. It would look like this:

redisManager.RedisCaches.Define("name")
                           .(Region and RG selection)
                           .WithStandardSku/WithBasicSku().....

redisManager.RedisPremiumCaches.Define("nameForPremium").
                           .(Region and RG selection)
                           .withShardCount(2)
                           .withPathcSchedule()......

What do you think?

@hovsepm hovsepm modified the milestones: v1.11, v1.12 May 31, 2018
@hovsepm hovsepm modified the milestones: v1.12, v1.13 Jun 22, 2018
hovsepm added a commit to Azure/azure-libraries-for-java that referenced this issue Jun 29, 2018
* Regenerated Redis with latest fixes.

* fix for Azure/azure-libraries-for-net#305

* Fix for Azure/azure-libraries-for-net#304

* fix for Azure/azure-libraries-for-net#303

* Fixed build break

* commit of the current dev state

* Added comments.

* Fixed stylecheck errors.

* fix of the fix

* Fixed the fix of the fix

* Fix the build break.

* Fix for PatchSchedule failures

* fixed style check breaks

* Fixed LinkedServers.

* Fixed build breaks.

* Re-recorded redis Azure test

* re-recorded Redis sample

* Fixed Beta since text.
@hovsepm
Copy link
Contributor

hovsepm commented Jul 16, 2018

Release 1.13 is out. Please upgrade your references.

@hovsepm hovsepm closed this as completed Jul 16, 2018
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

No branches or pull requests

2 participants