> On July 6, 2012, 9:52 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 375 > > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line375> > > > > Please use a finally() to do the decrement
The intention here is to decrement the resource count if an exception is being thrown on error. Are you suggesting that the entire if check needs to be put in in a try finally and resource count decremented on error? Can you clarify more on your comment. > On July 6, 2012, 9:52 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 380 > > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line380> > > > > Please use a finally() to do the decrement Same query as above. Can you clarify more on this comment. What additional purpose will adding a try finally around this code block do, if we are only throwing an exception in the if code block. > On July 6, 2012, 9:52 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 391 > > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line391> > > > > Please use the finally above to do the decrement > > decrement is already being done in catch exception. Any particular reason why a finally needs to be added to do a decrement. We only want to decrement on error and not otherwise. > On July 6, 2012, 9:52 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 497 > > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line497> > > > > Again this is boilerplate code. We are already in a finally code block and we want to decrement only if snapshot backup fails and it is in error state. > On July 6, 2012, 9:52 p.m., Nitin Mehta wrote: > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 1332 > > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line1332> > > > > If you go inside the function checkResourceLimit and > > incrementResourceCount you will see that they are synchronized by locking > > the rows so you don't require this. The intention is to synchronize across checkResourceLimit and incrementResourceCount function and not just within these two functions. For example, if resource limit is set to 5 for a domain and already the limit has reached 4. If two requests comes in from different accounts in the domain for creating a snapshot; if both of them are able to check the limit before either of them increments the count, we can hit a scenario where the limits can be exceeded. Am I missing something? - Devdeep ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5806/#review8930 ----------------------------------------------------------- On July 6, 2012, 12:43 p.m., deepti dohare wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5806/ > ----------------------------------------------------------- > > (Updated July 6, 2012, 12:43 p.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 > ----- > > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7 > > 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 > >
