ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@gridgain.com>
Subject Re: Fwd: Coding Guidelines: zookeeper IP finder and mqtt streamer
Date Mon, 28 Sep 2015 22:49:10 GMT
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.

D.

On Tue, Sep 29, 2015 at 12:41 AM, Konstantin Boudnik <cos@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 <raulk@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 <cos@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
> >>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message