Re: [ovirt-devel] Gluster Volume Snapshots - Feature review

Hi Juan, Thanks for the detailed comments. My comments inline. Thanks and Regards, Shubhendu On 12/11/2014 08:56 PM, Juan Hernández wrote: > On 12/11/2014 05:36 AM, Shubhendu Tripathi wrote: >> Hi Juan, >> >> Incorporated the review comments. >> Kindly have a look and let us know if everything looks well and good. >> >> Thanks and Regards, >> Shubhendu >> > Thanks for making the changes. I have some additional comments: > > 1. URL segments shouldn't have underscores. For example, you are > proposing URLs like this: > > /clusters/{cluster:id}/glustervolumes/{volume:id}/volume_snapshots > > The last component should be "volumesnapshots", without the underscore. > Note that on the other hand XML element should have underscores, like in > "<volume_snapshots>", that is correct. Sure. Will do the changes accordingly. > > 2. Try to avoid abbreviations. For example, instead of "whatever_params" > use "whatever_parameters", and instead of "scheduling_det" use > "scheduling_details", use "cron_expression" instead of "cronexpr", so > on. Sure. Will do the changes accordingly. > > 3. If possible the action to delete a snapshot shouldn't receive any > parameters, not even an empty "<action/>" element. Sure. Will do the changes accordingly. > > 4. The "schedulesnapshot" action should be modeled using REST style, not > an action. My understanding is that you plan to have for each volume a > set of rules that define when the snapshots will be created. This should > be implemented as a sub-collection of the volume, so that these rules > can be queried, added, modified and deleted: > > To query: > > GET ...{volume:id}/snapshotrules > <snapshot_rules> > <snapshot_rule id="..." href="..."> > <crontab_expression>...</crontab_expression> > </snapshot_rule> > ... > </snapshot_rules> > > To add: > > POST ...{volume:id{/snapshotrules > <snapshot_rule> > <crontab_expression>...</crontab_expression> > </snapshot_rule> > > To modify: > > PUT ...{volume:id}/snapshotrules/{snapshotrule:id} > <snapshot_rule> > <crontab_expression>...</crontab_expression> > </snapshot_rule> > > To delete (note that there is no body): > > DELETE ...{volume:id}/snapshotrules/{snapshotrule:id} > > If there will be only one of these rules per volume then you can model > them as attributes of the volume itself, without the sub-collection. At a time there would be only one rule for a volume, so as suggested it could be a volume attribute. Will model accordingly. > > 5. The "snapshotconfigs" should be modeled as an attribute of the > volume, not as an action. As above. > >> On 12/09/2014 07:23 PM, Shubhendu Tripathi wrote: >>> Thanks Juan for the comments. I would update the wiki accordingly >>> and send for confirmation. >>> >>> Regards >>> Shubhendu >>> >>> Sent from Samsung Mobile >>> >>> -------- Original message -------- >>> From: Juan Hernández <jhernand(a)redhat.com&gt; >>> Date: 09/12/2014 18:51 (GMT+05:30) >>> To: Shubhendu Tripathi >>> <shtripat@redhat.com&gt;,devel(a)ovirt.org,Michael Pasternak >>> <mpastern(a)redhat.com&gt; >>> Subject: Re: [ovirt-devel] Gluster Volume Snapshots - Feature review >>> On 12/04/2014 07:11 PM, Shubhendu Tripathi wrote: >>>> Hi Juan/Michael, >>>> >>>> This is a gentle reminder for the review of the REST api design >>>> for the >>>> below feature. >>>> We would be starting the REST development for the same soon >>>> (probably by >>>> Dec 2014 end). >>>> >>>> If there are specific comments, please pass it on. If no comments we >>>> would go ahead with the current design and implement accordingly. >>>> >>>> Request your time for this. >>>> >>>> Thanks and Regards, >>>> Shubhendu >>>> >>>> On 11/10/2014 12:22 PM, Shubhendu Tripathi wrote: >>>>> Hi All, >>>>> >>>>> Please help us to review the design of Gluster Volume Snapshots >>>>> in oVirt, >>>>> >>>>> Here are two design on wiki page >>>>> >>>>> General Feature Design >>>>> http://www.ovirt.org/Features/GlusterVolumeSnapshots >>>>> >>>>> Detailed Design >>>>> http://www.ovirt.org/Features/Design/GlusterVolumeSnapshots >>>>> >>>>> We target it in ovirt 3.6 release. >>>>> >>>>> Marked Juan/Michael specifically for REST review. >>>>> >>>>> Best Regards, >>>>> Shubhendu Tripathi >>> My comments about the RESTAPI: >>> >>> 1. You can't use the "snapshot" and "snapshots" XML elements, as those >>> are already in use for disk snapshots, and we don't have name space >>> support in the RESTAPI. You will have to use something different, for >>> example "gluster_volume_snapshots" and "gluster_volume_snapshot". >>> >>> 2. When adding a volume snapshot the name of the volume shouldn't be a >>> parameter, as that is implicit. Only the name and description of the >>> snapshot should be provided. >>> >>> 3. The operation to delete a snapshot should be performed on the >>> snapshot resource, not on the collection: >>> >>> DELETE >>> /clusters/{cluster:id}/glustervolumes/{volume:id}/snapshots/{snapshot:id} >>> >>> >>> Ideally this operation shouldn't receive any parameters (thus no >>> body). >>> If it does require parameters then they should be contained inside an >>> "action" element. >>> >>> 4. The operation to update the snapshot configuration should be the >>> PUT >>> operation of the volume, not a new "snapshotconfig" sub-resource, as >>> these kind of sub-resources aren't well supported by the SDKs and >>> the CLI. >>> >> _______________________________________________ >> Devel mailing list >> Devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/devel >> > _______________________________________________ Devel mailing list Devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel