Hi Yakov,
Thanks for the review.
Comments inline.
Aside from them: being honest, do you think Javadoc like the following
snippets are useful and provide any value? More so in a test case...
Snippet #1:
/**
* @param topics Topics.
* @param fromIdx From index.
* @param cnt Count.
* @param singleMsg Single message flag.
* @throws MqttException If failed.
*/
Snippet #2:
/**
* @throws Exception If failed.
*/
To me, they are redundant. And I'm concerned about imposing superfluous
Javadoc "just because", or due to aesthetics, rather than value.
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 Thu, Oct 22, 2015 at 11:52 AM, Yakov Zhdanov <yzhda...@apache.org> wrote:
> Hi Raul!
>
> Thank you very much for addressing my comments!
>
> I like the code however there are still a couple of points (I committed
> everything to ignite-1747):
>
> 1. Please take a look at
>
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-StringOutput
Yeah, I'm familiar with this rule and I thought I was complying with it.
Could you give me an example where the code is not compliant?
> 2. org.apache.ignite.stream.mqtt.MqttStreamer#connected - volatile write
> then read of most likely the same value - I changed it to return true
> literal.
> connected = true;
> return connected;
>
Agree.
> 3. blockUntilConnected - I would name this property syncConn
>
Why? To me "blockUntilConnected" is clearer and more explicit than
"syncConn", which sounds mystic to an ordinary user who's not familiar with
the community's abbreviation table. It's ok for a private member, but not
for a property. In fact, syncConn requires the user to have the Javadoc
handy, blockUntilConnected doesn't. So I prefer the latter.
> 4. Abbreviation rules violations - connected - conn, executor - exec,
> values - vals, count - cnt, message - msg
>
Ok, this is a rule... yeah. But you can imagine it's extremely hard to
memorise someone else's glossary. In my opinion, abbreviations should be
used only when needed, not by default. An abbreviation subtracts
readability for the general user – long-term Ignite developers are probably
used to these by now.
> 5. What is the point of "connected" member? Client has "isConnected()"? Is
> it similar to what you want to achieve?
>
I'll look at the implementation of MqttClient.isConnected(), but off the
top of my head, it seems it could be replaced, yes.
> 6. Race on stop - 1 thread calls stop and successfully exits the method,
> retrier is not stopped (awaitTermination has not been called - should it
> be?) and can connect client back - is this the case?
>
I'll have a look.
> 7. I think we use {@code } instead of <tt>...
>
Correct. Thanks.
On more point community should agree on - imports ordering and grouping. I
> will post another email.
>
This was already added a few weeks ago in the Coding rules.
>
> --Yakov
>
> 2015-10-20 20:39 GMT+03:00 Raul Kripalani <ra...@apache.org>:
>
> > Hi Yakov,
> >
> > I've had a few free cycles to fix the code style and Javadoc issues you
> > reported in the MQTT Streamer and I've pushed my changes to branch
> > ignite-1747.
> >
> > Would you mind having a look to see if it adapts better now?
> >
> > Thanks,
> >
> > *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
> >
>
