ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Raul Kripalani <r...@evosent.com>
Subject Re: Fwd: Coding Guidelines: zookeeper IP finder and mqtt streamer
Date Tue, 29 Sep 2015 12:01:27 GMT
Thanks, Cos. I'm glad we sorted this out. Sometimes a chat would be useful
to establish rapport.

Accusing someone of negligence is *extremely* serious and casts a very bad
image on the person being attributed with it. It's a destructive attribute.
No one here breaks the rules on purpose. It is unfair to tag anybody in
this community – and probably 99% of the ASF – with that concept. We should
always think right before thinking wrong. The benefit of the doubt goes a
long way.

To sum up, many factors have played a part here:

    (1) this was my first commit in a rigid community – and I already asked
for a review when I did it [1]. Unfortunately this feedback came in late
and through inappropriate broadcast channels. There's no reason to use a
broadcast list like dev@ for individual commit reviews. It's overkill.

    (2) the coding guidelines are imprecise and incongruent in some points.
I've made changes to the examples and added lots of TODOs: [2].

    (3) we have no tooling in place. The Ignite community is dominated by
the people who wrote the code and have become accustomed to reading it day
and night for years. They have a trained eye to detect weird stuff. They
need to understand that. Others like me do not and never will, because
we're involved in tens of projects, each with a different coding style.
That's why I insist in the importance of checkstyle. I'm bound to this
community now, but if I was a newcomer, this kind of witch hunt would have
deterred me from contributing to Ignite ever again. And as a current PMC
member, I'm very concerned about this attitude in general (but that's a
different topic).

    (4) there was wrong judgement when reading Git history, making the
issue seem much more severe than it was. The wiki speaks about squashing in
the context of a *Github pull request* [3], but this wasn't the case.
Moreover, there were never WIP commits on master.

    (5) And guess what? Merging to master without squashing feature
branches is happening all the time: [3]! I don't know why Yakov only caught
my mistake and not any others. He should clarify what differences are there
between my situation and others. Maybe there's something I'm missing.

[1]
http://apache-ignite-developers.2346864.n4.nabble.com/Maybe-review-my-first-commits-td3370.html
[2]
https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=57901455&selectedPageVersions=15&selectedPageVersions=14
[3]
https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-1.GitHubpull-request
[4] https://imgur.com/991Aa4X

*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 Tue, Sep 29, 2015 at 12:05 PM, Konstantin Boudnik <cos@apache.org> wrote:

> Dmitriy has pointed to me why there's a incorrect perception of me going
> after
> Raul ;) I haven't even noticed the last sentence of the original email.
> And I
> am sorry about sending the email too fast. What I was trying to do is to
> make
> a trivial statement: there's a mistake that needs to be fixed - let's do
> just
> that and move on.
>
> Cos
>
> On Tue, Sep 29, 2015 at 03:00AM, Konstantin Boudnik wrote:
> > Thank you you for confirming my point: there was a mistake and it needs
> to be
> > corrected. End of story. But instead of simply fixing it and moving on,
> we are
> > spending hours x 50 people on reading and writing long emails arguing
> about
> > imaginary semantical differences.
> >
> > There's no need to be emotional about who said what: I deserve the
> benefits of
> > the doubt as well despite being a "former mentor of the project"
> whatever the
> > hell it means. I am not dismissing the value of your contributions to
> this
> > community: I welcome and appreciate your efforts! Neither have I
> targeted you
> > nor put your on the stand - you did it to yourself. If you don't like
> > something you think I addressed to you - send me a private email and
> explain
> > that I was a jerk and hurt your feelings: no need to make a public
> display of
> > potential nothingness. Would I choose to listen to it or not - is a
> separate
> > matter altogether: I have the same right to not read or accept something
> that
> > another person has wrote.
> >
> > Let's move on. Best,
> >   Cos
> >
> > On Tue, Sep 29, 2015 at 01:16AM, Raul Kripalani wrote:
> > > There has been no negligence, Cos! People are human and make mistakes.
> End
> > > of the story.
> > >
> > > Bringing such negative verbiage to the community helps in nothing.
> > > Everybody is doing their best, I'd like to think so.
> > >
> > > In fact, you have shifted the conversation away from the actual topic
> at
> > > hand. So thanks.
> > >
> > > A suggestion: "Benefit of the doubt" is a powerful practice and keeps
> us
> > > away from errors in judgement.
> > >
> > > With regards your list of questions, may I ask you to re-read your
> initial
> > > message. Don't make me explain what's obvious, mate.
> > >
> > > Cheers.
> > > On 28 Sep 2015 23:41, "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