ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: Coding Guidelines: zookeeper IP finder and mqtt streamer
Date Mon, 28 Sep 2015 22:31:51 GMT
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 <yzhdanov@apache.org> wrote:

> Guys,
>
> I have just reviewed the code and have some comments. 1-2 are very serious
> from my point of view.
>
> 1. Code is in master. Did anyone checked tests on TC? Moreover, are there
> suites for those tests?
> 2. It seems that work on streamer has been done directly in master. I see
> WIP commits, but I think I should not. As agreed finished work should be
> committed as a single set of changes.
> 3. I see unused variable
> - org.apache.ignite.stream.mqtt.MqttStreamer#cachedLogPrefix
> 4. Unused import - import com.google.common.base.Joiner;
> 5. Code and javadocs lines exceed 120 chars restriction.
> 6. Plenty of javadocs issues - absence, multiline "inheritdoc", etc.
> 7. Spacing is not correct - in ignite codebase logical blocks are separated
> with blank line.
> 8. There should always be a blank line at the end of each file.
> 9. retrier vs retryier issue.
>
> Who is in charge for this code? Raul, Val? Can anyone fix my comments?
>
> I would also ask everyone (even committers) not to commit to master without
> doing review with another committer.
>
> Here is the link to Ignite's coding guidelines -
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines. Feel
> free to suggest and discuss edits if anything does not seem valid to you.
>
> Thanks!
>
> --Yakov
>

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