Mailing-List: contact dev-help@ignite.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@ignite.apache.org
MIME-Version: 1.0
In-Reply-To: <20150928125327.GO19314@boudnik.org>
References: 
 <CAGcMBHjoATU7nw__QtxD4fyr4j=iq7p9ogEP8skFA7RoGs7fZg@mail.gmail.com>
 <20150928125327.GO19314@boudnik.org>
From: Raul Kripalani <raulk@apache.org>
Date: Mon, 28 Sep 2015 15:39:51 +0100
Message-ID: 
 <CAKoNANv_x9T5Xe6pHMs5=x-5Q5SSfpraa=70fygFaepverLk9Q@mail.gmail.com>
Subject: Fwd: Coding Guidelines: zookeeper IP finder and mqtt streamer
To: dev@ignite.apache.org
Content-Type: multipart/alternative; boundary=001a11c345bcb53af70520cfacd8

--001a11c345bcb53af70520cfacd8
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 =E2=80=93 more so given that you're a f=
ormer
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=C3=BAl 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 C=
TR
> 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, styl=
e
> checks, etc. And someone else needs to deal with it to unblock her's futu=
re
> 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 shoul=
d
> 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 the=
re
> > suites for those tests?
> > 2. It seems that work on streamer has been done directly in master. I s=
ee
> > WIP commits, but I think I should not. As agreed finished work should b=
e
> > 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 yo=
u.
> >
> > Thanks!
> >
> > --Yakov
>

--001a11c345bcb53af70520cfacd8--