> On Aug. 22, 2012, 9:11 p.m., Nitin Mehta wrote: > > server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java, line 310 > > <https://reviews.apache.org/r/5806/diff/3/?file=141357#file141357line310> > > > > By changing the functionality of checkResourceLimit you are risking it > > for all the resource creation in the CS. > > Have you tested for them ? > > > > Instead of touching the existing functions I advise you to rewrite it > > all together and slowly all the owners will move their code to using your > > functions. > > deepti dohare wrote: > Here we are not changing the functionality of checkresourceLimit. To > avoid repetition of the code, we created a new method > checkResourceLimitforAccount, which is same as checkresourcelimit without > locks, such that it can be called in checkAndIncrementResourceCount. > > Tested for resources: instances, volume, snapshots and templates.
Wht i meant is changing fn name is not the right way Suffixing 2 means this is the same function but the next version and people should start using it just like we have product versions This relates to more maintainable code as well Also I don't want the existing functions to be touched simply bcz all the resource creation depends on them and this requires a lot of testing with all the edge cases ironed out. What you should do is call the snapshot creating through this and do a thorough testing Write a comment for this function so that the owners themselves would start migrating to the new function you will write. - Nitin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5806/#review10636 ----------------------------------------------------------- On Sept. 8, 2012, 9:50 a.m., deepti dohare wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5806/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2012, 9:50 a.m.) > > > Review request for cloudstack. > > > Description > ------- > > Change: > 1. Before creating the snapshot, we synchronized checkResourcelimit to allow > the users to create the snapshot and increment the resource count. > 2. Depending on the failure of snapshot creation/ backup, we are decrementing > the resource count. > > > This addresses bug CS-15430. > > > Diffs > ----- > > api/src/com/cloud/user/ResourceLimitService.java 98dfc11 > server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java 7f446d7 > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 8ae8ddc > > Diff: https://reviews.apache.org/r/5806/diff/ > > > Testing > ------- > > Steps to verify: > 1.Login as admin, set snapshot limit '3' for a user account > 2.login as user, create a VM1 with data volume > 3.trigger 3 create snapshot command from the above data volume, succeeded > 4.create one more snapshot, failed, "maximum limit exceeded for account user" > > > Thanks, > > deepti dohare > >
