Coding Guidelines: zookeeper IP finder and mqtt streamer

[yzhdanov]

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

[Konstantin Boudnik-2]

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

[Raul Kripalani]

Hey guys,

I have little time now, but before this discussion escalates...

#1 Yakov, how exactly are you reading the Git history? WIP commits were
never on master, they were in a feature branch which was merged into master
on this commit:

    Commit: 88acd318b84ce3bff8c061bb34718e0e5f7127fb [88acd31]
    Parents: 421a5234b5, 296dd6e7d8
    Author: Raul Kripalani <[hidden email]>
    Date: 21 Sep 2015 17:26:04 WEST

    IGNITE-535 Merge MQTT Streamer into master.


Screenshot of the graph here: https://imgur.com/6vy0Vc4.

#2 About TC tests, please see this discussion:
http://apache-ignite-developers.2346864.n4.nabble.com/Maybe-review-my-first-commits-td3370.html
.

#3 About formatting: yes there may be some problems. To the trained eye
they are very easy to spot, especially if you spend most of your time in
the Ignite codebase. But as more people join the team from other walks of
life, we need a better system:

  (a) the Coding Guidelines are not meticulous enough for the level of
auditing the community is doing; I get the impression that subjective bias
is playing a part here, e.g. the "logical blocks" comment -- can you
provide an objective definition accepted by industry?

  (b) we have no tooling; only a IntelliJ formatting definition, which on
the one hand is incoherent (e.g. it tries to turn single-line Javadoc into
multiline) and secondly, it is only valid for IntelliJ users (aren't there
more IDEs out there?).

  (c) since we have no tooling in place, you are expecting a human to
review every single character meticulously; this is inefficient and
discourages people from contributing.

  (d) for the people who are so concerned with this level of detail, would
you consider writing a checkstyle definition? It'll provide an objective
basis, and we can plug it into the build and make it fail. That'll be the
end of the story ;-)

By the way, I'm sure you are reviewing outdated code because Joiner *is*
used.

#4 While we are on this, can we please discuss stuff like this:
https://imgur.com/991Aa4X. Why doesn't the team use the --rebase option
when pulling? Also, we need to remember that branch names disappear when
the branch is deleted, therefore the commits lose their context (ticket,
feature, etc.) if the message does not carry it. You can see how the commit
log looks to another person in the screenshot.

P.S.: Will reply to your individual formatting comments later, but retrier
vs retryier is intentional. Retryier is the library (wrong spelling),
retrier are my variable names (correct spelling).

Regards,
Raúl.

On Mon, Sep 28, 2015 at 1:53 PM, Konstantin Boudnik <[hidden email]> 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
>

[Sergi]

Agree about checkstyle. I myself do mistakes even after more than 3 years
on this code base.
Checkstyle is a nice tool, works for many IDEs. For example H2 database
requires
incoming patches to conform provided checkstyle config, I use it right from
Eclipse.

Sergi

2015-09-28 16:25 GMT+03:00 Raul Kripalani <[hidden email]>:

> Hey guys,
>
> I have little time now, but before this discussion escalates...
>
> #1 Yakov, how exactly are you reading the Git history? WIP commits were
> never on master, they were in a feature branch which was merged into master
> on this commit:
>
>     Commit: 88acd318b84ce3bff8c061bb34718e0e5f7127fb [88acd31]
>     Parents: 421a5234b5, 296dd6e7d8
>     Author: Raul Kripalani <[hidden email]>
>     Date: 21 Sep 2015 17:26:04 WEST
>
>     IGNITE-535 Merge MQTT Streamer into master.
>
>
> Screenshot of the graph here: https://imgur.com/6vy0Vc4.
>
> #2 About TC tests, please see this discussion:
>
> http://apache-ignite-developers.2346864.n4.nabble.com/Maybe-review-my-first-commits-td3370.html
> .
>
> #3 About formatting: yes there may be some problems. To the trained eye
> they are very easy to spot, especially if you spend most of your time in
> the Ignite codebase. But as more people join the team from other walks of
> life, we need a better system:
>
>   (a) the Coding Guidelines are not meticulous enough for the level of
> auditing the community is doing; I get the impression that subjective bias
> is playing a part here, e.g. the "logical blocks" comment -- can you
> provide an objective definition accepted by industry?
>
>   (b) we have no tooling; only a IntelliJ formatting definition, which on
> the one hand is incoherent (e.g. it tries to turn single-line Javadoc into
> multiline) and secondly, it is only valid for IntelliJ users (aren't there
> more IDEs out there?).
>
>   (c) since we have no tooling in place, you are expecting a human to
> review every single character meticulously; this is inefficient and
> discourages people from contributing.
>
>   (d) for the people who are so concerned with this level of detail, would
> you consider writing a checkstyle definition? It'll provide an objective
> basis, and we can plug it into the build and make it fail. That'll be the
> end of the story ;-)
>
> By the way, I'm sure you are reviewing outdated code because Joiner *is*
> used.
>
> #4 While we are on this, can we please discuss stuff like this:
> https://imgur.com/991Aa4X. Why doesn't the team use the --rebase option
> when pulling? Also, we need to remember that branch names disappear when
> the branch is deleted, therefore the commits lose their context (ticket,
> feature, etc.) if the message does not carry it. You can see how the commit
> log looks to another person in the screenshot.
>
> P.S.: Will reply to your individual formatting comments later, but retrier
> vs retryier is intentional. Retryier is the library (wrong spelling),
> retrier are my variable names (correct spelling).
>
> Regards,
> Raúl.
>
> On Mon, Sep 28, 2015 at 1:53 PM, Konstantin Boudnik <[hidden email]>
> 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
> >
>

[Raul Kripalani]

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 <[hidden email]> 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
>

[Vishal Garg]

Raul,
I still think it is valid to have a peer review and if someone is trying and spending time to keep code style consistent, it is a good thing. And It should not matter who gets to fixing the style issues until we have check styles or a much improved process for reviewing.

Code review comments should not be taken personally. End of the day it is what gets to the community, the more +1s we get as a better and consistent code styles, the better Ignite would do as open source.

Now, I do agree we had GG contribute most of it, so code styling conventions come partly from there. But, if we have some comments on styling we could continuously improve it as an Ignite community.
Vishal

Sent from my iPhone

> On Sep 28, 2015, at 7:39 AM, Raul Kripalani <[hidden email]> 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 <[hidden email]> 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
>>

[dsetrakyan]

Yakov,

I think it is best to provide the code review comments in the Jira ticket,
and not here. Also, Jira ticket has information on who authored the code as
well (don't think we should be calling names out on the dev list).

Can you please reopen the ticket and provide the link to this discussion
there?

Thanks,
D.

On Mon, Sep 28, 2015 at 2:31 PM, Yakov Zhdanov <[hidden email]> 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
>

[Konstantin Boudnik-2]

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 <[hidden email]> 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 <[hidden email]>
>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
>>

[Dmitriy Setrakyan]

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 <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>
> >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
> >>
>

[Raul Kripalani]

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" <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>
> >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
> >>
>

[dsetrakyan]

On Tue, Sep 29, 2015 at 12:49 AM, Dmitriy Setrakyan <[hidden email]
> 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 <[hidden email]>
> 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 <[hidden email]>
>> 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 <[hidden email]>
>> >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
>> >>
>>
>
>

[dsetrakyan]

One more point about empty lines. I have reviewed our empty line policy and
I don't think I can do a better job describing it. The explanation is
pretty accurate.

However, the main problem with our empty line policy is that, although the
resulting code looks very neat, the policy is just too complex. It would be
nice if someone with correct code style settings could create a correct
auto-format configuration for IDEA and Eclipse. Any volunteers?

D.

On Tue, Sep 29, 2015 at 2:44 AM, Dmitriy Setrakyan <[hidden email]>
wrote:

>
>
> On Tue, Sep 29, 2015 at 12:49 AM, Dmitriy Setrakyan <
> [hidden email]> 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 <[hidden email]>
>> 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 <[hidden email]>
>>> 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 <[hidden email]>
>>> >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
>>> >>
>>>
>>
>>
>

[yzhdanov]

> #1 Yakov, how exactly are you reading the Git history? WIP commits were
> never on master, they were in a feature branch which was merged into
master
> on this commit:

You can check the history of the streamer. I see more than one entry.
Probably, that was mistake on merge - commits were not squashed. Please
don't take it personally. I am talking to everyone - please squash commits
when merge to master! This is described here -
https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-Forcommitters

> #2 About TC tests, please see this discussion:
>
http://apache-ignite-developers.2346864.n4.nabble.com/Maybe-review-my-first-commits-td3370.html

Again, you committed to master without checking TC :) No?
https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-Forcontributors
- you omitted "check TC step".
I think if TC not working, nothing should be committed to master unless
community agrees that commit is safe. As a workaround we still have ability
to check the patch internally (if Ignite TC not working) and we could do it
for you. Btw, TC should be recovered (at least partially) -
http://204.14.53.153/

> (a) the Coding Guidelines are not meticulous enough for the level of
> auditing the community is doing; I get the impression that subjective bias
> is playing a part here, e.g. the "logical blocks" comment -- can you
> provide an objective definition accepted by industry?

please check code around and you can easily catch what is meant here. Take
a look, for example, at GridNearAtomicUpdateFuture

> (d) for the people who are so concerned with this level of detail, would
> you consider writing a checkstyle definition? It'll provide an objective
> basis, and we can plug it into the build and make it fail. That'll be the
> end of the story ;-)

I doubt if I will ever do that. Let's wait for someone else :-), till that
moment we will be doing peer reviews =)

> By the way, I'm sure you are reviewing outdated code because Joiner *is*
used.

I had warning in IDE. It could be problem on my side. Now I see it is used.

> Will reply to your individual formatting comments later..
Please don't forget about
- unused members
- empty javadocs

> #4 While we are on this, can we please discuss stuff like this:
> https://imgur.com/991Aa4X. Why doesn't the team use the --rebase option
> when pulling? Also, we need to remember that branch names disappear when
> the branch is deleted, therefore the commits lose their context (ticket,
> feature, etc.) if the message does not carry it. You can see how the
commit
> log looks to another person in the screenshot.

Should we care about this if we squash on pushing to master?

Thanks!

--Yakov

[Konstantin Boudnik-2]

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" <[hidden email]> 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 <[hidden email]>
> > 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 <[hidden email]>
> > >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
> > >>
> >

[Konstantin Boudnik-2]

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" <[hidden email]> 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 <[hidden email]>
> > > 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 <[hidden email]>
> > > >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
> > > >>
> > >

[Raul Kripalani-2]

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 <[hidden email]> 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" <[hidden email]> 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 <
> [hidden email]>
> > > > 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 <[hidden email]
> >
> > > > >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
> > > > >>
> > > >
>

[dsetrakyan]

On Tue, Sep 29, 2015 at 2:01 PM, Raul Kripalani <[hidden email]> wrote:

    (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).
>

Raul, I don't think I agree with you here. Somehow you are making it sound
that if we don't have the "right" tooling, then we should not try to
enforce coding guidelines. That would be a disaster in my view.

I think that given the current situation, providing styling comments during
reviews is the right way to go. All the feedback we have received so far is
that people generally appreciate how nice Ignite code looks, and gladly
accept the feedback on style from more experienced committers.

Also, take a look at some other replies on this subject, especially the
comments that came from the new community members.

D.

[Roman Shaposhnik]

On Tue, Sep 29, 2015 at 3:01 PM, Raul Kripalani <[hidden email]> wrote:
> 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.

I've been there and I understand where you're coming from. Different words
have different level of gravity in different languages and cultures.

FWIW: I didn't read it as invoking catastrophic ramifications and/or doubting
integrity of the person. But that's *my* reading. Somebody who, at the end of
the day, speaks English fluently but alas as a second language. I do apologize
if it made if you feel uncomfortable.

I think the key takeaway for all of us who chimed in on this thread is
to be extremely
appreciative of the fact that we all come from different backgrounds
and cultures
and can interpret things differently. Lets give each other a bit of a breathing
room and a benefit of the doubt.

Thanks,
Roman.

[Anton Vinogradov]

Raul,

I've fixed most of TODOs you created.
Seems that indentation policy with multi-line parameter descriptions
already defined: "multiline comments in Javadoc tags should be indented by
4 or 5 characters ...".

Please check my changes:
https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=57901455&selectedPageVersions=16&selectedPageVersions=15



On Tue, Sep 29, 2015 at 3:28 PM, Roman Shaposhnik <[hidden email]>
wrote:

> On Tue, Sep 29, 2015 at 3:01 PM, Raul Kripalani <[hidden email]> wrote:
> > 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.
>
> I've been there and I understand where you're coming from. Different words
> have different level of gravity in different languages and cultures.
>
> FWIW: I didn't read it as invoking catastrophic ramifications and/or
> doubting
> integrity of the person. But that's *my* reading. Somebody who, at the end
> of
> the day, speaks English fluently but alas as a second language. I do
> apologize
> if it made if you feel uncomfortable.
>
> I think the key takeaway for all of us who chimed in on this thread is
> to be extremely
> appreciative of the fact that we all come from different backgrounds
> and cultures
> and can interpret things differently. Lets give each other a bit of a
> breathing
> room and a benefit of the doubt.
>
> Thanks,
> Roman.
>

[Konstantin Boudnik-2]

On Fri, Oct 02, 2015 at 04:26PM, Anton Vinogradov wrote:
> Raul,
>
> I've fixed most of TODOs you created.
> Seems that indentation policy with multi-line parameter descriptions
> already defined: "multiline comments in Javadoc tags should be indented by
> 4 or 5 characters ...".

That's a bit weird. Java style recommends 8, I believe. And a number of the
projects in Apache uses 2-4-4, which I personally find a better looking and
compact compared to the Java style. But 5?

Cos

> Please check my changes:
> https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=57901455&selectedPageVersions=16&selectedPageVersions=15
>
>
>
> On Tue, Sep 29, 2015 at 3:28 PM, Roman Shaposhnik <[hidden email]>
> wrote:
>
> > On Tue, Sep 29, 2015 at 3:01 PM, Raul Kripalani <[hidden email]> wrote:
> > > 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.
> >
> > I've been there and I understand where you're coming from. Different words
> > have different level of gravity in different languages and cultures.
> >
> > FWIW: I didn't read it as invoking catastrophic ramifications and/or
> > doubting
> > integrity of the person. But that's *my* reading. Somebody who, at the end
> > of
> > the day, speaks English fluently but alas as a second language. I do
> > apologize
> > if it made if you feel uncomfortable.
> >
> > I think the key takeaway for all of us who chimed in on this thread is
> > to be extremely
> > appreciative of the fact that we all come from different backgrounds
> > and cultures
> > and can interpret things differently. Lets give each other a bit of a
> > breathing
> > room and a benefit of the doubt.
> >
> > Thanks,
> > Roman.
> >