> On April 10, 2014, 5:19 p.m., Nitin Mehta wrote: > > engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java, > > line 134 > > <https://reviews.apache.org/r/20210/diff/2/?file=554427#file554427line134> > > > > Why do we need to update the old entry and not create a new one ? We > > should handle listTemplate not returning the destroyed result for the > > template ?
Hi Min, in case of template deletion and recopying we update the same entry in template zone ref, So i wanted to make the behaviour consistent even in case of entries in the template_store_ref when dealing with re copying of templates. I am not sure which one to follow as both have advantages and disadvantages. but what ever is done i think it should be consistent. so since the idea is to create new entries always. i will create a bug for template_zone_ref case and change the current patch to do the same. > On April 10, 2014, 5:19 p.m., Nitin Mehta wrote: > > engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java, > > line 141 > > <https://reviews.apache.org/r/20210/diff/2/?file=554427#file554427line141> > > > > Changing the state not using state machine is bad practice. Hi Min, I was updating a already destroyed template so had to set it manually as state machine will not come into picture for cases like these. > On April 10, 2014, 5:19 p.m., Nitin Mehta wrote: > > engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java, > > line 286 > > <https://reviews.apache.org/r/20210/diff/2/?file=554428#file554428line286> > > > > method name says its zone but then you are passing the image store ? Hi Min, will change this to markAsDestroyedByTemplateStore. > On April 10, 2014, 5:19 p.m., Nitin Mehta wrote: > > server/src/com/cloud/template/HypervisorTemplateAdapter.java, line 341 > > <https://reviews.apache.org/r/20210/diff/2/?file=554429#file554429line341> > > > > The comment doesnt match what you are doing. The comment was not about what the function is doing, it was about the intent of that snippet of the code. will change the comment to be more explicit about this. > On April 10, 2014, 5:19 p.m., Nitin Mehta wrote: > > server/src/com/cloud/template/HypervisorTemplateAdapter.java, line 342 > > <https://reviews.apache.org/r/20210/diff/2/?file=554429#file554429line342> > > > > Why is this required ? > > This should happen via state machine and not like this. Are you seeing > > the destroyed not being set ? Hi Min, There are two fields which can be set to destroyed int the template_store_ref. one is the state and the other is the destroyed flag. Again this is confusing as I am not sure why do we have a destroyed flag and a destroyed state. I think one should be enough. - bharat ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20210/#review40034 ----------------------------------------------------------- On April 10, 2014, 3:04 p.m., bharat kumar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20210/ > ----------------------------------------------------------- > > (Updated April 10, 2014, 3:04 p.m.) > > > Review request for cloudstack, Kishan Kavala and Min Chen. > > > Bugs: CLOUDSTACK-6377 > https://issues.apache.org/jira/browse/CLOUDSTACK-6377 > > > Repository: cloudstack-git > > > Description > ------- > > List templates returns destroyed temples when copying the same template after > deleting. > https://issues.apache.org/jira/browse/CLOUDSTACK-6377 > > > Diffs > ----- > > > engine/api/src/org/apache/cloudstack/storage/datastore/db/TemplateDataStoreDao.java > 048ce22 > > engine/storage/src/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java > d9a5164 > > engine/storage/src/org/apache/cloudstack/storage/image/db/TemplateDataStoreDaoImpl.java > f5140e0 > server/src/com/cloud/template/HypervisorTemplateAdapter.java 963aec9 > > Diff: https://reviews.apache.org/r/20210/diff/ > > > Testing > ------- > > Tested on 4.2 > > > Thanks, > > bharat kumar > >
