Coding Guidelines: zookeeper IP finder and mqtt streamer

[Branko Čibej]

On 29.09.2015 00:41, Konstantin Boudnik wrote:
> Hmm...
>
> Negligence, n. : the trait of neglecting responsibilities and lacking concern
> syn : omission, oversight

"Negligence" usually means continued and repeated (non-)action. In that
respect it's an extremely negative label to use; you're effectively
accusing someone of being an incompetent lazy b**rd.

The "synonyms" are not synonymous because both "omission" and
"oversight" imply a on-time event.


-- Brane

> 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