> On Jul 14, 2015, at 1:47 AM, Owen Synge <osynge@xxxxxxxx> wrote: > > On 07/09/2015 09:58 PM, Travis Rhoden wrote: >> >>> On Jul 9, 2015, at 4:59 AM, Owen Synge <osynge@xxxxxxxx> wrote: >>> >>> On 07/09/2015 12:46 PM, John Spray wrote: >>>> Owen, >>> >>> Hi John, >>> >>> thanks for your reasonable mail. >>> >>>> Please can you say what your overall goal is with recent ceph-deploy >>>> patches? >>> >>> To give ceph-deploy code features we have in downstream ceph-deploy to >>> upstream. >>> >>> All I am trying to do is reduce copious code duplication during the >>> process of up streaming. It seems such duplication is wanted for reasons >>> I am yet to understand. >>> >>> I only understand that due to architectural bad practices such as this >>> one in how to do a basic facade class. >>> >>> I made my downstream code OS preferred directory neutral, and init >>> system neutral using abstractions. >> >> I’ve added a new set of comments in https://github.com/ceph/ceph-deploy/pull/317 that hopefully clarifies my position. > > A lot thank you. > >> I am *not* in favor of copious code duplication. > > Good. I am glad you clarified this in both the pull request and in this > thread. > >> I *am* in favor of not introducing new paradigms or architectures in the stable 1.5.x series. The 1.5.x series is packaged and supported downstream, and introducing major changes will complicate that. ceph-deploy has been in a state where the main changes we have been taking are: (1) bug/security fixes (2) new features to keep up with Ceph features (like civetweb-based RGW) (3) fixes to make user help and execution output more clear. > > Good. This was my major issue, that led to this thread. > >> A major refactoring (I would consider anything that moves away from the current facade paradigm to be major in 1.5.x) doesn’t fit into that. However, ceph-deploy is not dead or maintenance only, so long as there is community interest. The upstream installer developers (of which I am one) and Red Hat (also me again) are looking at alternatives for more robust installation solutions that do not involve ceph-deploy. That is also a motivation for not making major changes, like those proposed in https://github.com/ceph/ceph-deploy/pull/320. I am concerned that it will make long-term maintenance more complicated. > > I quiet understand redhat and upstreams perspective that ceph-deploy is > NOT a good plan going forward as a "robust installation solutions" for > large clusters. I also favour using more conventional cluster management > solutions on large and production clusters. > > To me, when working on new functionality, such as rgw with fastcgi or > civetweb is not re factoring. > > My only remaining fear is that you consider a major re-factoring is for > me minor changes for new development. > > This probably only reflects our different attitudes to the compromise > between incremental improvement Vs consistency. Probably a very true observation. > >>>> Whether a given structure is right or wrong is really a >>>> function of the overall direction. If you are aiming to make >>>> ceph-deploy into an extensible framework that can do things in parallel, >>>> then you need to say so, and that's a bigger conversation about whether >>>> that's a reasonable goal for a tool which has so far made a virtue of >>>> maintaining modest ambitions. >>> >>> Understood and irrelevant. >>> >>> As I stated in the email you replied to the issue of only one instance >>> existing which prevents processing more than one node at a time, or >>> even, connecting to two servers at the same time (so you can query >>> update a second node) can be worked around, by importing the modules as >>> objects, but issue (2) the structural "orange" issues are far more >>> serious, leading to structural artifacts of a badly implemented facade >>> pattern interfere with the complete code base. >>> >>>> Along these lines, I noticed that you recently submitted a pull request >>>> to ceph-deploy that added a dependency on SQLAlchemy, and several >>>> hundred lines of extra boilerplate code -- this kind of thing is >>>> unlikely to get a warm reception without a stronger justification for >>>> the extra weight. While I don't know how related that is to the points >>>> you're making in this post, but it certainly inspires some curiosity >>>> about where you're going with this. >>> >>> It was presented as a possible way to make a MVC model. >>> >>> It was only a POC, that this way of doing things could do all that is >>> required. >>> >>> I pulled this POC as it was told to me that no matter what I said MVC or >>> structural changes to ceph-deploy in any way where to be opposed by >>> former developers of ceph-deploy. >> >> We’ve been trying to only accept code into ceph-deploy that has a corresponding bugzilla or Ceph tracker bug referenced. I mention this because it helps us answer the question “what problem does this solve”? We haven’t enforced to all contributors in the community, but we’re trying to be more stringent about it. The code you’ve proposed in https://github.com/ceph/ceph-deploy/pull/320 seems to mainly address creating required RGW pools. I’d like to see a bug opened against that so that we can see if it is indeed something that needs to be fixed. It may very well be that this is something that RGW is supposed to be doing itself (just talking out loud again). > > I think here we see the reason why I pulled my discussion point. > > You considered it a bug fix / contribution to the code. > > I considered it a discussion point on code quality and best practice. > > If you are happy to consider this as a discussion point on code quality > and best practice (as I thought I had explained the day before in the > meeting), then I am happy to start discussion in a meeting again. I do think the waters got muddied. I take some blame for that. It is much easier to evaluate the code in light of the exact problem it is trying to fix rather than seeing it as a model for something different. You did make it clear that it was for discussion only. We do this all the time, actually. Usually by prefixing Pull Request titles with “DNM” — “Do Not Merge”. Though sometimes that can also be “I want you to review this early, even though it’s not ready”. > >>> I might have presented an alternative MVC but since the objections where >>> to MVC and also SQLAlchemy when ever it would strengthen arguments >>> against MVC, without even waiting to discuss positives or negatives. >>> >>> I did not want all my "bug fix" patches for ceph-deploy not beign >>> reviewed due to peoples seeing my name, and fearing letting in any >>> patches which I did into the code base. >> >> I can only speak for myself here, but I would not do that and I don’t think my colleagues would either. It would not lead to a very friendly community if any of us would rather have bugs in the code than review perfectly good patches that fix real problems. > > Now we are clear that I wanted to discuss improving the code style, > (particularly for new functionality in rgw) > > As I understand it I should not in future: > > 1) Provide demo code on discussion points. Slides are a better idea. I don’t think any of us want slides. :) DNM pull requests are fine. > 2) Discuss implementation, best practice and design at the same time. > 3) Make discussion points when I have any open pull requests for fear > that others may mix these independent issues. This is hard to say. I certainly don’t think that it is bad to have pull request open when trying to talk about major architectural changes. Each PR should stand on its own merit, and be reviewed individually. What is probably helpful is if we make the intent of each PR clear — as in this is a simple bug fix, this is a discussion point / idea, etc. > 4) Ever (even if explicitly stated as was the case) make a pull request > for any other purposes than intending to merge with the code base. Again, I wouldn’t go that far. I think most of speak code better than slides. When proposing something major, it’s always a risk. You have to fully flesh out the idea to make it work and convince others that it’s good, but that takes time and effort that you are guaranteed to see pay off. If enough discussion can happen early on to see if the idea will be well received, that’s better for everyone. I think you did try to do that, and the change in design pattern was not well received. However in this context, I know I evaluated it in terms of *could* I merge this right now, not “would this be worthwhile down the road”. > > I should in future: > > 1) Raise all proposals in written form. > 2) Raise all proposals in written form that is public such as this > mailing list first and never in a private meeting if I expect any > discussion. Open discussion is best, for sure. The more eyes the better. When we chatted in our standup, it was a difficult forum as it was not the correct audience. Those daily standups pretty much never will be right forum, as they are much more of the form “what did you do yesterday, what are you going to do today, and is there anything blocking your progress?”. They are meant to be fast paced and keep the group up to date with everyone else’s activities. > 3) Always assume the reader is going to conflate every concept in the > discussion unless explicitly written down in public. > 4) Contact the author of "unhelpful" comments for clarification and give > them at least 2 working days to answer as they may have wrote and I may > have read different things from what is intended by the author or > interpreted by me the reader of "unhelpful" comments. > > > Your suggestions for how to avoid such friction and misunderstanding in > future are very welcome. I think we are on the right track right here — sticking to ceph-devel the mailing list, and IRC. As for comments you deem unhelpful, asking for clarification does seem like the way to go. > >>>> I had not seen your wip_services_abstraction branch before, I've just >>>> taken a quick look now. More comments would probably have made it >>>> easier to read, as would following PEP8. >>> >>> Good review points that would have encouraged me we where moving >>> forwards not backwards, and suggesting everythign has to be part of the >>> "boat anchor" facade we already have. >>> >>>> I don't think there's anything >>>> problematic about having a class that knows how to start and stop a >>>> service, but I don't know what comments you've received elsewhere (there >>>> aren't any on the PR). >> >> All the comments relevant to the class abstraction are in that PR. Other conversations have taken place on IRC, but that’s been around the proposal for more of an MVC style that incorporates SQL Alchemy, and that is more related to PR #320. I’ve clarified my review on #317 to say that I like the classes for the init systems as well, but want to see them used a bit differently and to not use the higher level abstraction above them. > > You have explicitly stated that you are open to such suggestions as > introducing classes and objects to the code base of ceph-deploy. > > I hope you now understand that I misunderstood some statements you have > since withdrawn that led me to believe that you and others in the team > opposed any "change" in the code style ever. > > I now understand I misunderstood the rules and practices you used for > communication. > > Following this very helpful thread I will start a new thread on MVC. If > that goes well I will then start a discussion thread on implementation. I think that sounds like a prudent plan. > > Best regards > > Owen -- 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
