On Tue, Sep 29, 2015 at 12:49 AM, Dmitriy Setrakyan <dsetrak...@gridgain.com > wrote:
> Cos, > > I think Raul's point was that coding guidelines are not very clear. I > think Raul thought that he was following the coding guidelines. I don't > think "negligence" is a fair word to describe this. > > In my view, we have a couple of omissions in the process on Wiki that need > to be spelled out clearly: > > - semantic blocks should be described better in the coding guidelines > - we should clearly state that master should always be release-ready in > the lira process > - the best practice is to go through review in the ticket ignite-1234 > branch, instead of in master. > > I will try to fix it and send it here for review. > Guys, I think we have a very confusing Wiki. We have GIT Process page and Jira Process pages. Both of them describe some part of the same process. I think they should be combined into 1 page with GIT and JIRA sections. We can call this page GIT and JIRA Process. Also, I could not find any place where we mention that contributors may create a GitHub pull request. Since we accept pull requests, that section definitely needs to be added. Thoughts? > > D. > > On Tue, Sep 29, 2015 at 12:41 AM, Konstantin Boudnik <c...@apache.org> > wrote: > >> Hmm... >> >> Negligence, n. : the trait of neglecting responsibilities and lacking >> concern >> syn : omission, oversight >> >> Doesn't sound catastrophic in my vocabulary, really. Does this >> > case of negligence and needs to be addressed accordingly. >> translate to "should face a firing squad without a trial of his peers"? >> Have I anywhere pointed a finger at you or anyone else? Or attacked >> someone? Why are you all upset and defensive about it? >> >> Cos >> >> On September 28, 2015 7:39:51 AM PDT, Raul Kripalani <ra...@apache.org> >> wrote: >> >Cos, your language seems too harsh for the situation. >> > >> >No one here is committing negligence. The explanation is simple: people >> >aren't perfect. >> > >> >Now, let's take a step back and see the big picture. Around 95% of the >> >commits in this project are by GridGain personnel (check git shortlog >> >-s >> >-n) who have spent months/years working on this codebase during their >> >daily >> >job. Their eyes are accustomed to this code style and naturally they'll >> >spot oddities in a twitch. It's obvious. >> > >> >For newer people, we don't even have checkstyle nor decent facilities >> >for >> >newer people to spot formatting issues quickly. Because, surprise! The >> >issues that Yakov spotted are simply of formatting. The code is >> >functional >> >and much better tested than other streamers and IP Finders. Other >> >streamers >> >have 1 test, this streamer has 9 unit tests! Look at the code. >> >Furthermore, >> >Yakov seems to have made a mistake reading the Git commit history. >> >There >> >were never WIP commits on master. >> > >> >So may I ask you to stop using catastrophic vocabulary. The situation >> >is >> >not catastrophic, it's simply improvable. >> > >> >Now, as an ASF member, I ask you to recognise that unaffiliated >> >volunteers >> >like me bring diversity to the project that's otherwise dominated by a >> >company. You should appreciate that – more so given that you're a >> >former >> >mentor. I do this for the fun, and attacks like yours take the fun out >> >of >> >it. Have a look again at this project's team composition and, for those >> >people not affiliated to GridGain, try to find when their last commit >> >was... Then you'll see what I mean. >> > >> >P.S.: I did not merge the ZK IP Finder myself and I'm assuming that >> >Valentin will want to comment. >> > >> >Regards, >> > >> >*Raúl Kripalani* >> >PMC & Committer @ Apache Ignite, Apache Camel | Integration, Big Data >> >and >> >Messaging Engineer >> >http://about.me/raulkripalani | >> >http://www.linkedin.com/in/raulkripalani >> >http://blog.raulkr.net | twitter: @raulvk >> > >> >On Mon, Sep 28, 2015 at 1:53 PM, Konstantin Boudnik <c...@apache.org> >> >wrote: >> > >> >> Are these official guidelines that are worked-out and communicated by >> >> community? Basically, were they made clear when the project went on >> >the CTR >> >> model? I presume it is/was looking at the wikipage. Hence >> >non-sticking >> >> to them is a case of negligence and needs to be addressed >> >accordingly. >> >> >> >> I would also want to highlight the other side of such negligence: by >> >> dumping >> >> semi-baked code to the master one creates a burden for the rest of >> >the >> >> community as the code degrades in quality, potentially breaks tests, >> >style >> >> checks, etc. And someone else needs to deal with it to unblock her's >> >future >> >> progress. And that's brings forward another point that Brane and I >> >were >> >> making on a few occasions: in the CTR communities you need to invite >> >in >> >> people >> >> with great deal of attention to how they work with others. Are they >> >> respecting >> >> others' time and effort? Are they good citizens of the community? And >> >on, >> >> and >> >> on. >> >> >> >> Another purely technically matter: master isn't a trash can. Master >> >should >> >> be >> >> close to releasable at any given point of time. WIP stuff doesn't >> >belong to >> >> master, that's what the dev and integration branches are for. >> >> >> >> Cos >> >> >> >> On Mon, Sep 28, 2015 at 03:31PM, Yakov Zhdanov wrote: >> >> > Guys, >> >> > >> >> > I have just reviewed the code and have some comments. 1-2 are very >> >> serious >> >> > from my point of view. >> >> > >> >> > 1. Code is in master. Did anyone checked tests on TC? Moreover, are >> >there >> >> > suites for those tests? >> >> > 2. It seems that work on streamer has been done directly in master. >> >I see >> >> > WIP commits, but I think I should not. As agreed finished work >> >should be >> >> > committed as a single set of changes. >> >> > 3. I see unused variable >> >> > - org.apache.ignite.stream.mqtt.MqttStreamer#cachedLogPrefix >> >> > 4. Unused import - import com.google.common.base.Joiner; >> >> > 5. Code and javadocs lines exceed 120 chars restriction. >> >> > 6. Plenty of javadocs issues - absence, multiline "inheritdoc", >> >etc. >> >> > 7. Spacing is not correct - in ignite codebase logical blocks are >> >> separated >> >> > with blank line. >> >> > 8. There should always be a blank line at the end of each file. >> >> > 9. retrier vs retryier issue. >> >> > >> >> > Who is in charge for this code? Raul, Val? Can anyone fix my >> >comments? >> >> > >> >> > I would also ask everyone (even committers) not to commit to master >> >> without >> >> > doing review with another committer. >> >> > >> >> > Here is the link to Ignite's coding guidelines - >> >> > >> >https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines. >> >> Feel >> >> > free to suggest and discuss edits if anything does not seem valid >> >to you. >> >> > >> >> > Thanks! >> >> > >> >> > --Yakov >> >> >> > >
