Another version of PR is available addressing the latest three comments. On Fri, Jun 3, 2016 at 5:33 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: > Just updated the pull request with all fixes. Please review one more time. > > On Wed, May 25, 2016 at 6:01 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: >> Here is the link to my pull request with operations on consistency >> groups only: create cg, remove cg, list cgs. >> >> https://github.com/ceph/ceph/pull/9333 >> >> On Wed, May 25, 2016 at 5:53 PM, Jason Dillaman <jdillama@xxxxxxxxxx> wrote: >>> Perfect. I prefer to have the PR's commits reflect the clean >>> end-state of the change instead of having multiple "tweak" commits >>> fixing code review comments. Therefore, a rebase to address the >>> comment in its associated comment and force push is the desired >>> process. >>> >>> On Wed, May 25, 2016 at 5:34 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: >>>> I this case I case create a separate pull request for consistency >>>> group's operations only(create, remove, list). >>>> Another pull request will be: add image to cg, remove image, list images. >>>> >>>> One more question. How do you prefer comments to be addressed, as an >>>> additional commit to the pull request or amend the corresponding >>>> commit? >>>> >>>> V. >>>> >>>> On Tue, May 24, 2016 at 10:30 PM, Jason Dillaman <jdillama@xxxxxxxxxx> wrote: >>>>> The current PR is at approximately 2500 lines -- it would be nice to >>>>> have a PR under 1000 changed lines in a perfect world. The trouble >>>>> with larger PR is that after you address comments, I have to re-read >>>>> it all again. The smaller the PR, the faster it can be reviewed and >>>>> merged. If changes depend on another PR, you can always >>>>> non-fast-forward merge the other PR branch into your dependent PR >>>>> branch so that it's clear what needs to be reviewed. >>>>> >>>>> On Tue, May 24, 2016 at 6:42 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: >>>>>> Yes, I'm working on splitting it into small logically separated commits. >>>>>> My current PR is for CRUD operations only(create, remove, add image, >>>>>> remove image, show info). >>>>>> Do you want even a smaller PR or this one is small enough? >>>>>> >>>>>> Thanks, >>>>>> V. >>>>>> >>>>>> On Mon, May 23, 2016 at 7:39 PM, Jason Dillaman <jdillama@xxxxxxxxxx> wrote: >>>>>>> Definitely want to ensure that it cleanly merges with master. I would >>>>>>> also request, if at all possible, that you break it into individual >>>>>>> PRs of concrete sub-tasks for implementing consistency groups for ease >>>>>>> of review. Have individual commits for each step of implementing the >>>>>>> task would also help (i.e. squash related commits, fix style issues or >>>>>>> bugs in the commit that introduced them, etc). >>>>>>> >>>>>>> On Mon, May 23, 2016 at 6:43 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: >>>>>>>> Never mind. I just realized that it will be easier to build it on top >>>>>>>> of the latest master in any case. >>>>>>>> >>>>>>>> On Mon, May 23, 2016 at 8:32 AM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: >>>>>>>>> Jason, >>>>>>>>> >>>>>>>>> Do you prefer pull requests to be rebased on top of the latest master >>>>>>>>> or should I keep it where I started the development? >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> V. >>>>>>>>> >>>>>>>>> On Mon, May 16, 2016 at 8:48 AM, Jason Dillaman <jdillama@xxxxxxxxxx> wrote: >>>>>>>>>> On Thu, May 12, 2016 at 10:45 PM, Victor Denisov <vdenisov@xxxxxxxxxxxx> wrote: >>>>>>>>>>> Jason, >>>>>>>>>>> >>>>>>>>>>> Do you have any opinion regarding deleting images that are in a >>>>>>>>>>> consistency group? >>>>>>>>>>> >>>>>>>>>>> Should we delete them as well as the references in the consistency >>>>>>>>>>> group they belong to or should we prohibit deleting images that are in >>>>>>>>>>> a consistency group? >>>>>>>>>>> >>>>>>>>>>> V. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Right now, if an image has a snapshot we required you to remove all >>>>>>>>>> snapshots before removing the image. Along those lines, if an image >>>>>>>>>> is in a consistency group and the consistency group has snapshots, the >>>>>>>>>> user wouldn't be able to remove the image since it has snapshots nor >>>>>>>>>> should the user be able to remove the snapshots associated with the >>>>>>>>>> consistency group. In this case, the user would be forced to >>>>>>>>>> dissociate the image from the group before attempting to delete it. >>>>>>>>>> Therefore, just to keep the actions consistent, you might as well >>>>>>>>>> force the user to dissociate an image from the consistency group even >>>>>>>>>> if the image doesn't have snapshots. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Jason >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Jason >>>>> >>>>> >>>>> >>>>> -- >>>>> Jason >>> >>> >>> >>> -- >>> Jason -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
