You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Raul Kripalani <ra...@apache.org> on 2015/10/20 19:39:04 UTC
MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
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
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Raul Kripalani <ra...@apache.org>.
Hey Denis,
Yep, I've been using the Ignite code style for IntelliJ since the
beginning.
It's ok for indentation, whitespacing, line breaks, etc. But things too
specific like abbreviations, string outputs, Javadoc conventions like using
{@code ...}, etc. are not covered.
checkstyle is a potential solution to automate the audit. We'll definitely
need to implement custom checks, but it's worth it. Earlier I saw a comment
in a ticket from another user asking for a Sonar-like report of their
mistakes... So at least I'm not alone in my frustration!
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 8:59 PM, Denis Magda <dm...@gridgain.com> wrote:
> Raul,
>
> It wasn't easy for me to get into the way of following Ignite coding
> guidelines. So you're not alone :)
>
> However one day I found "ignite/idea/ignite_codeStyle.xml" file and set it
> in Intellij IDEA settings. Since that time coding guidelines has never
> worried me again. Probably this will help you as well.
>
>
> --
> Denis
>
>
> On 10/22/2015 10:50 PM, Dmitriy Setrakyan wrote:
>
>> Raul,
>>
>> Thanks for your contribution!
>>
>> I want to address the coding guidelines comments, as they keep coming up
>> again and again. I do agree that coding guidelines may not be ideal, and
>> may seem redundant at times. However, they make Ignite code look
>> consistent. If you try to change them now, even if it seems in a better
>> direction, the overall impact on the code will be negative, as the old
>> code
>> will remain inconsistent with new one.
>>
>> I am also noticing that every discussion about coding guidelines is about
>> a
>> matter of taste. We will never all agree on the taste, so my preference
>> would be just to follow the guidelines as they are.
>>
>> I also think that we should relax the coding guidelines for the
>> contributors (non-committers) as long as they get fixed by the committer
>> who does the final merge. This will make contributions easier (I will
>> start
>> a separate thread about it).
>>
>> D.
>>
>> On Thu, Oct 22, 2015 at 12:22 PM, Raul Kripalani <ra...@apache.org>
>> wrote:
>>
>> 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 <yz...@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
>>>>>
>>>>>
>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Denis Magda <dm...@gridgain.com>.
Raul,
It wasn't easy for me to get into the way of following Ignite coding
guidelines. So you're not alone :)
However one day I found "ignite/idea/ignite_codeStyle.xml" file and set
it in Intellij IDEA settings. Since that time coding guidelines has
never worried me again. Probably this will help you as well.
--
Denis
On 10/22/2015 10:50 PM, Dmitriy Setrakyan wrote:
> Raul,
>
> Thanks for your contribution!
>
> I want to address the coding guidelines comments, as they keep coming up
> again and again. I do agree that coding guidelines may not be ideal, and
> may seem redundant at times. However, they make Ignite code look
> consistent. If you try to change them now, even if it seems in a better
> direction, the overall impact on the code will be negative, as the old code
> will remain inconsistent with new one.
>
> I am also noticing that every discussion about coding guidelines is about a
> matter of taste. We will never all agree on the taste, so my preference
> would be just to follow the guidelines as they are.
>
> I also think that we should relax the coding guidelines for the
> contributors (non-committers) as long as they get fixed by the committer
> who does the final merge. This will make contributions easier (I will start
> a separate thread about it).
>
> D.
>
> On Thu, Oct 22, 2015 at 12:22 PM, Raul Kripalani <ra...@apache.org> wrote:
>
>> 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 <yz...@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
>>>>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Dmitriy Setrakyan <ds...@apache.org>.
Raul,
Thanks for your contribution!
I want to address the coding guidelines comments, as they keep coming up
again and again. I do agree that coding guidelines may not be ideal, and
may seem redundant at times. However, they make Ignite code look
consistent. If you try to change them now, even if it seems in a better
direction, the overall impact on the code will be negative, as the old code
will remain inconsistent with new one.
I am also noticing that every discussion about coding guidelines is about a
matter of taste. We will never all agree on the taste, so my preference
would be just to follow the guidelines as they are.
I also think that we should relax the coding guidelines for the
contributors (non-committers) as long as they get fixed by the committer
who does the final merge. This will make contributions easier (I will start
a separate thread about it).
D.
On Thu, Oct 22, 2015 at 12:22 PM, Raul Kripalani <ra...@apache.org> wrote:
> 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 <yz...@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
> > >
> >
>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Konstantin Boudnik <co...@apache.org>.
On Fri, Oct 23, 2015 at 09:22PM, Raul Kripalani wrote:
> Dmitriy,
>
> The first step is to write a complete specification for a coding style.
> It's still incomplete from what I see.
>
> It's inadmissible that new rules pop up in an ad hoc manner during reviews,
> based on a single person's judgement.
>
> If it's not documented in the guidelines, it *cannot be enforced*, neither
> to contributors nor to committers. No one is a mind reader and no one is
> going to spend the time to go through all of Ignite's codebase to infer
> "unwritten rules" and apply them. That's what the guidelines are for.
+1 - very good points and we need to make sure the expectation are simple and
expressed _upfront_: not because "we used to do it, look at this file" or "I
don't think this code style is pretty".
> @Yakov — could you please enhance the guidelines ASAP?
>
> @Dmitriy — just to be clear again. I'm not complaining about following the
> guidelines. My intentions are (1) complete coverage in the guidelines and
> (2) to challenge some rules which just provide fluff with arguable value
> (IMHO). Never have I questioned the necessity of a clean and uniform
> codebase. This is primordial in successful OSS.
>
> Regards,
> Raúl.
> On 23 Oct 2015 21:01, "Dmitriy Setrakyan" <ds...@apache.org> wrote:
>
> > Gents,
> >
> > First of all, can I ask why this review is being done on the dev list
> > instead of in the Jira? All we are doing is splitting the communication
> > about the same feature into multiple places. I believe we have all agreed
> > in the past that Jira is the proper place for it.
> >
> > Now, I have not done the review, so I cannot comment on any type of
> > concurrency or bug fixing. As far as error messages starting with the word
> > “Failed to …”, I can see a potential reason for it. For example, I can do a
> > grep in the log for the word “Failed to” and find many error messages
> > easily. To Raul’s point, it is not mentioned in the coding guidelines, so
> > it should be up to the project veterans to make sure that the coding
> > guidelines are properly updated (I am sure we are going to keep finding
> > such omissions here and there until we get it perfect).
> >
> > Generally speaking, the purpose of the coding guidelines is to make the
> > code more consistent. We have been getting a lot of praise for the quality
> > of Ignite code, and I believe that it is mainly due to the strict coding
> > guidelines rules.
> >
> > I will start another discussion thread on the dev list asking everyone in
> > the community, contributors or committers, whether the current guidelines
> > should be relaxed.
> >
> > D.
> >
> > On Fri, Oct 23, 2015 at 9:56 AM, Raul Kripalani <ra...@apache.org> wrote:
> >
> > > Hey Yakov,
> > >
> > > About the exception messages, come on, you have to admit you are making
> > up
> > > rules as we go ;-)
> > >
> > > Nowhere in the coding guidelines does it say that all exception messages
> > > have to start with "Failed to...". In fact, there are hundreds if not
> > > thousands of Exception messages in the existing Ignite codebase.
> > Examples:
> > >
> > > throw new IgniteException("Spring application context resource is not
> > > injected.");
> > > throw new IgniteException("Cache store was not properly
> > initialized.");
> > > throw new IgniteClientDisconnectedException(reconnectFut, "Client
> > node
> > > disconnected: " + gridName);
> > > throw new GridClientClosedException("Client was closed (no public
> > > methods of client can be used anymore).");
> > >
> > > About the full stop at the end, there is a reference, but the way it's
> > > written is incorrect (make each token start with upper case and end with
> > a
> > > dot?!?!)
> > >
> > > Note that all prefix, postfix, name, value should follow English
> > > grammar, in particular, start with upper case and end with the dot '.'.
> > >
> > > Moreover, not even the examples on the Wiki are following that rule.
> > > Neither are lots of exceptions in the current code base... Please review
> > > the whole situation as it seems that double standards are being applied
> > for
> > > old vs. newly contributed code.
> > >
> > > Also, I noticed that you changed all of my inline comments to upper case
> > > and finished them with a dot. Could you please add this rule to the Wiki?
> > >
> > > Finally, I have pushed a new version of the streamer that checks the
> > > connection state by calling MqttStreamer#isConnected. Changed the retrier
> > > accordingly and added 2 unit tests.
> > >
> > > I don't consider syncConn is an appropriate nor user-friendly
> > denominator.
> > >
> > > 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 Fri, Oct 23, 2015 at 8:12 AM, Yakov Zhdanov <yz...@gridgain.com>
> > > wrote:
> > >
> > > > Raul,
> > > >
> > > > My comments are below.
> > > >
> > > > 2015-10-22 22:22 GMT+03:00 Raul Kripalani <ra...@apache.org>:
> > > >
> > > > > 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.
> > > > > */
> > > > >
> > > >
> > > > This makes sense for me - this makes code looks consistent.
> > > >
> > > >
> > > > >
> > > > > 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.
> > > > >
> > > >
> > > > This is not so redundant. If some special condition should be
> > mentioned -
> > > > go ahead and put it to javadoc. This javadoc shows at least that
> > > developer
> > > > has not just forget to put the description and there are no special
> > > > conditions to mention.
> > > >
> > > > Moreover, I thought I saw couple of places where javadoc stated
> > @throws,
> > > > but method did not throw any exception. It seems I fixed that. Can you
> > > > please take a look at my changes?
> > > >
> > > > Another point - javadoc to streamer public method
> > > (isBlockUntilConnected())
> > > > was completely omitted. I hope everyone agrees that configurational
> > > methods
> > > > should be documented.
> > > >
> > > >
> > > > >
> > > > > 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 <yzhdanov@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?
> > > > >
> > > >
> > > > throw new IgniteException("Exception while initializing
> > > > MqttStreamer", t); - does not start with "Failed to.." and does not end
> > > > with dot
> > > > throw new IgniteException("Attempted to stop an already
> > > stopped
> > > > MQTT Streamer");
> > > > throw new IgniteException("Exception while stopping
> > > > MqttStreamer", t);
> > > >
> > > > + 'MqttStreamer' vs 'MQTT Streamer' inconsistency. I like 'MQTT
> > Streamer'
> > > > more.
> > > >
> > > > I thought I changed those lines. Can you please take a look at my
> > > changes?
> > > >
> > > >
> > > > >
> > > > >
> > > > > > 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.
> > > > >
> > > >
> > > > This name does not state that connection is blocked on start. Moreover,
> > > > this adds no value as connection may disappear right after start and
> > > > streamer should recover connection in background. Maybe we can remove
> > it?
> > > >
> > > >
> > > > >
> > > > > > 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.
> > > >
> > > >
> > > > That's fine then.
> > > >
> > > >
> > > > Yakov
> > > >
> > >
> >
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Raul Kripalani <ra...@apache.org>.
Dmitriy,
The first step is to write a complete specification for a coding style.
It's still incomplete from what I see.
It's inadmissible that new rules pop up in an ad hoc manner during reviews,
based on a single person's judgement.
If it's not documented in the guidelines, it *cannot be enforced*, neither
to contributors nor to committers. No one is a mind reader and no one is
going to spend the time to go through all of Ignite's codebase to infer
"unwritten rules" and apply them. That's what the guidelines are for.
@Yakov — could you please enhance the guidelines ASAP?
@Dmitriy — just to be clear again. I'm not complaining about following the
guidelines. My intentions are (1) complete coverage in the guidelines and
(2) to challenge some rules which just provide fluff with arguable value
(IMHO). Never have I questioned the necessity of a clean and uniform
codebase. This is primordial in successful OSS.
Regards,
Raúl.
On 23 Oct 2015 21:01, "Dmitriy Setrakyan" <ds...@apache.org> wrote:
> Gents,
>
> First of all, can I ask why this review is being done on the dev list
> instead of in the Jira? All we are doing is splitting the communication
> about the same feature into multiple places. I believe we have all agreed
> in the past that Jira is the proper place for it.
>
> Now, I have not done the review, so I cannot comment on any type of
> concurrency or bug fixing. As far as error messages starting with the word
> “Failed to …”, I can see a potential reason for it. For example, I can do a
> grep in the log for the word “Failed to” and find many error messages
> easily. To Raul’s point, it is not mentioned in the coding guidelines, so
> it should be up to the project veterans to make sure that the coding
> guidelines are properly updated (I am sure we are going to keep finding
> such omissions here and there until we get it perfect).
>
> Generally speaking, the purpose of the coding guidelines is to make the
> code more consistent. We have been getting a lot of praise for the quality
> of Ignite code, and I believe that it is mainly due to the strict coding
> guidelines rules.
>
> I will start another discussion thread on the dev list asking everyone in
> the community, contributors or committers, whether the current guidelines
> should be relaxed.
>
> D.
>
> On Fri, Oct 23, 2015 at 9:56 AM, Raul Kripalani <ra...@apache.org> wrote:
>
> > Hey Yakov,
> >
> > About the exception messages, come on, you have to admit you are making
> up
> > rules as we go ;-)
> >
> > Nowhere in the coding guidelines does it say that all exception messages
> > have to start with "Failed to...". In fact, there are hundreds if not
> > thousands of Exception messages in the existing Ignite codebase.
> Examples:
> >
> > throw new IgniteException("Spring application context resource is not
> > injected.");
> > throw new IgniteException("Cache store was not properly
> initialized.");
> > throw new IgniteClientDisconnectedException(reconnectFut, "Client
> node
> > disconnected: " + gridName);
> > throw new GridClientClosedException("Client was closed (no public
> > methods of client can be used anymore).");
> >
> > About the full stop at the end, there is a reference, but the way it's
> > written is incorrect (make each token start with upper case and end with
> a
> > dot?!?!)
> >
> > Note that all prefix, postfix, name, value should follow English
> > grammar, in particular, start with upper case and end with the dot '.'.
> >
> > Moreover, not even the examples on the Wiki are following that rule.
> > Neither are lots of exceptions in the current code base... Please review
> > the whole situation as it seems that double standards are being applied
> for
> > old vs. newly contributed code.
> >
> > Also, I noticed that you changed all of my inline comments to upper case
> > and finished them with a dot. Could you please add this rule to the Wiki?
> >
> > Finally, I have pushed a new version of the streamer that checks the
> > connection state by calling MqttStreamer#isConnected. Changed the retrier
> > accordingly and added 2 unit tests.
> >
> > I don't consider syncConn is an appropriate nor user-friendly
> denominator.
> >
> > 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 Fri, Oct 23, 2015 at 8:12 AM, Yakov Zhdanov <yz...@gridgain.com>
> > wrote:
> >
> > > Raul,
> > >
> > > My comments are below.
> > >
> > > 2015-10-22 22:22 GMT+03:00 Raul Kripalani <ra...@apache.org>:
> > >
> > > > 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.
> > > > */
> > > >
> > >
> > > This makes sense for me - this makes code looks consistent.
> > >
> > >
> > > >
> > > > 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.
> > > >
> > >
> > > This is not so redundant. If some special condition should be
> mentioned -
> > > go ahead and put it to javadoc. This javadoc shows at least that
> > developer
> > > has not just forget to put the description and there are no special
> > > conditions to mention.
> > >
> > > Moreover, I thought I saw couple of places where javadoc stated
> @throws,
> > > but method did not throw any exception. It seems I fixed that. Can you
> > > please take a look at my changes?
> > >
> > > Another point - javadoc to streamer public method
> > (isBlockUntilConnected())
> > > was completely omitted. I hope everyone agrees that configurational
> > methods
> > > should be documented.
> > >
> > >
> > > >
> > > > 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 <yzhdanov@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?
> > > >
> > >
> > > throw new IgniteException("Exception while initializing
> > > MqttStreamer", t); - does not start with "Failed to.." and does not end
> > > with dot
> > > throw new IgniteException("Attempted to stop an already
> > stopped
> > > MQTT Streamer");
> > > throw new IgniteException("Exception while stopping
> > > MqttStreamer", t);
> > >
> > > + 'MqttStreamer' vs 'MQTT Streamer' inconsistency. I like 'MQTT
> Streamer'
> > > more.
> > >
> > > I thought I changed those lines. Can you please take a look at my
> > changes?
> > >
> > >
> > > >
> > > >
> > > > > 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.
> > > >
> > >
> > > This name does not state that connection is blocked on start. Moreover,
> > > this adds no value as connection may disappear right after start and
> > > streamer should recover connection in background. Maybe we can remove
> it?
> > >
> > >
> > > >
> > > > > 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.
> > >
> > >
> > > That's fine then.
> > >
> > >
> > > Yakov
> > >
> >
>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Dmitriy Setrakyan <ds...@apache.org>.
Gents,
First of all, can I ask why this review is being done on the dev list
instead of in the Jira? All we are doing is splitting the communication
about the same feature into multiple places. I believe we have all agreed
in the past that Jira is the proper place for it.
Now, I have not done the review, so I cannot comment on any type of
concurrency or bug fixing. As far as error messages starting with the word
“Failed to …”, I can see a potential reason for it. For example, I can do a
grep in the log for the word “Failed to” and find many error messages
easily. To Raul’s point, it is not mentioned in the coding guidelines, so
it should be up to the project veterans to make sure that the coding
guidelines are properly updated (I am sure we are going to keep finding
such omissions here and there until we get it perfect).
Generally speaking, the purpose of the coding guidelines is to make the
code more consistent. We have been getting a lot of praise for the quality
of Ignite code, and I believe that it is mainly due to the strict coding
guidelines rules.
I will start another discussion thread on the dev list asking everyone in
the community, contributors or committers, whether the current guidelines
should be relaxed.
D.
On Fri, Oct 23, 2015 at 9:56 AM, Raul Kripalani <ra...@apache.org> wrote:
> Hey Yakov,
>
> About the exception messages, come on, you have to admit you are making up
> rules as we go ;-)
>
> Nowhere in the coding guidelines does it say that all exception messages
> have to start with "Failed to...". In fact, there are hundreds if not
> thousands of Exception messages in the existing Ignite codebase. Examples:
>
> throw new IgniteException("Spring application context resource is not
> injected.");
> throw new IgniteException("Cache store was not properly initialized.");
> throw new IgniteClientDisconnectedException(reconnectFut, "Client node
> disconnected: " + gridName);
> throw new GridClientClosedException("Client was closed (no public
> methods of client can be used anymore).");
>
> About the full stop at the end, there is a reference, but the way it's
> written is incorrect (make each token start with upper case and end with a
> dot?!?!)
>
> Note that all prefix, postfix, name, value should follow English
> grammar, in particular, start with upper case and end with the dot '.'.
>
> Moreover, not even the examples on the Wiki are following that rule.
> Neither are lots of exceptions in the current code base... Please review
> the whole situation as it seems that double standards are being applied for
> old vs. newly contributed code.
>
> Also, I noticed that you changed all of my inline comments to upper case
> and finished them with a dot. Could you please add this rule to the Wiki?
>
> Finally, I have pushed a new version of the streamer that checks the
> connection state by calling MqttStreamer#isConnected. Changed the retrier
> accordingly and added 2 unit tests.
>
> I don't consider syncConn is an appropriate nor user-friendly denominator.
>
> 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 Fri, Oct 23, 2015 at 8:12 AM, Yakov Zhdanov <yz...@gridgain.com>
> wrote:
>
> > Raul,
> >
> > My comments are below.
> >
> > 2015-10-22 22:22 GMT+03:00 Raul Kripalani <ra...@apache.org>:
> >
> > > 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.
> > > */
> > >
> >
> > This makes sense for me - this makes code looks consistent.
> >
> >
> > >
> > > 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.
> > >
> >
> > This is not so redundant. If some special condition should be mentioned -
> > go ahead and put it to javadoc. This javadoc shows at least that
> developer
> > has not just forget to put the description and there are no special
> > conditions to mention.
> >
> > Moreover, I thought I saw couple of places where javadoc stated @throws,
> > but method did not throw any exception. It seems I fixed that. Can you
> > please take a look at my changes?
> >
> > Another point - javadoc to streamer public method
> (isBlockUntilConnected())
> > was completely omitted. I hope everyone agrees that configurational
> methods
> > should be documented.
> >
> >
> > >
> > > 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 <yz...@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?
> > >
> >
> > throw new IgniteException("Exception while initializing
> > MqttStreamer", t); - does not start with "Failed to.." and does not end
> > with dot
> > throw new IgniteException("Attempted to stop an already
> stopped
> > MQTT Streamer");
> > throw new IgniteException("Exception while stopping
> > MqttStreamer", t);
> >
> > + 'MqttStreamer' vs 'MQTT Streamer' inconsistency. I like 'MQTT Streamer'
> > more.
> >
> > I thought I changed those lines. Can you please take a look at my
> changes?
> >
> >
> > >
> > >
> > > > 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.
> > >
> >
> > This name does not state that connection is blocked on start. Moreover,
> > this adds no value as connection may disappear right after start and
> > streamer should recover connection in background. Maybe we can remove it?
> >
> >
> > >
> > > > 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.
> >
> >
> > That's fine then.
> >
> >
> > Yakov
> >
>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Raul Kripalani <ra...@apache.org>.
Hey Yakov,
About the exception messages, come on, you have to admit you are making up
rules as we go ;-)
Nowhere in the coding guidelines does it say that all exception messages
have to start with "Failed to...". In fact, there are hundreds if not
thousands of Exception messages in the existing Ignite codebase. Examples:
throw new IgniteException("Spring application context resource is not
injected.");
throw new IgniteException("Cache store was not properly initialized.");
throw new IgniteClientDisconnectedException(reconnectFut, "Client node
disconnected: " + gridName);
throw new GridClientClosedException("Client was closed (no public
methods of client can be used anymore).");
About the full stop at the end, there is a reference, but the way it's
written is incorrect (make each token start with upper case and end with a
dot?!?!)
Note that all prefix, postfix, name, value should follow English
grammar, in particular, start with upper case and end with the dot '.'.
Moreover, not even the examples on the Wiki are following that rule.
Neither are lots of exceptions in the current code base... Please review
the whole situation as it seems that double standards are being applied for
old vs. newly contributed code.
Also, I noticed that you changed all of my inline comments to upper case
and finished them with a dot. Could you please add this rule to the Wiki?
Finally, I have pushed a new version of the streamer that checks the
connection state by calling MqttStreamer#isConnected. Changed the retrier
accordingly and added 2 unit tests.
I don't consider syncConn is an appropriate nor user-friendly denominator.
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 Fri, Oct 23, 2015 at 8:12 AM, Yakov Zhdanov <yz...@gridgain.com>
wrote:
> Raul,
>
> My comments are below.
>
> 2015-10-22 22:22 GMT+03:00 Raul Kripalani <ra...@apache.org>:
>
> > 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.
> > */
> >
>
> This makes sense for me - this makes code looks consistent.
>
>
> >
> > 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.
> >
>
> This is not so redundant. If some special condition should be mentioned -
> go ahead and put it to javadoc. This javadoc shows at least that developer
> has not just forget to put the description and there are no special
> conditions to mention.
>
> Moreover, I thought I saw couple of places where javadoc stated @throws,
> but method did not throw any exception. It seems I fixed that. Can you
> please take a look at my changes?
>
> Another point - javadoc to streamer public method (isBlockUntilConnected())
> was completely omitted. I hope everyone agrees that configurational methods
> should be documented.
>
>
> >
> > 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 <yz...@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?
> >
>
> throw new IgniteException("Exception while initializing
> MqttStreamer", t); - does not start with "Failed to.." and does not end
> with dot
> throw new IgniteException("Attempted to stop an already stopped
> MQTT Streamer");
> throw new IgniteException("Exception while stopping
> MqttStreamer", t);
>
> + 'MqttStreamer' vs 'MQTT Streamer' inconsistency. I like 'MQTT Streamer'
> more.
>
> I thought I changed those lines. Can you please take a look at my changes?
>
>
> >
> >
> > > 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.
> >
>
> This name does not state that connection is blocked on start. Moreover,
> this adds no value as connection may disappear right after start and
> streamer should recover connection in background. Maybe we can remove it?
>
>
> >
> > > 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.
>
>
> That's fine then.
>
>
> Yakov
>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Yakov Zhdanov <yz...@gridgain.com>.
Raul,
My comments are below.
2015-10-22 22:22 GMT+03:00 Raul Kripalani <ra...@apache.org>:
> 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.
> */
>
This makes sense for me - this makes code looks consistent.
>
> 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.
>
This is not so redundant. If some special condition should be mentioned -
go ahead and put it to javadoc. This javadoc shows at least that developer
has not just forget to put the description and there are no special
conditions to mention.
Moreover, I thought I saw couple of places where javadoc stated @throws,
but method did not throw any exception. It seems I fixed that. Can you
please take a look at my changes?
Another point - javadoc to streamer public method (isBlockUntilConnected())
was completely omitted. I hope everyone agrees that configurational methods
should be documented.
>
> 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 <yz...@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?
>
throw new IgniteException("Exception while initializing
MqttStreamer", t); - does not start with "Failed to.." and does not end
with dot
throw new IgniteException("Attempted to stop an already stopped
MQTT Streamer");
throw new IgniteException("Exception while stopping
MqttStreamer", t);
+ 'MqttStreamer' vs 'MQTT Streamer' inconsistency. I like 'MQTT Streamer'
more.
I thought I changed those lines. Can you please take a look at my changes?
>
>
> > 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.
>
This name does not state that connection is blocked on start. Moreover,
this adds no value as connection may disappear right after start and
streamer should recover connection in background. Maybe we can remove it?
>
> > 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.
That's fine then.
Yakov
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Raul Kripalani <ra...@apache.org>.
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 <yz...@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
> >
>
Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Posted by Yakov Zhdanov <yz...@apache.org>.
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
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;
3. blockUntilConnected - I would name this property syncConn
4. Abbreviation rules violations - connected - conn, executor - exec,
values - vals, count - cnt, message - msg
5. What is the point of "connected" member? Client has "isConnected()"? Is
it similar to what you want to achieve?
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?
7. I think we use {@code } instead of <tt>...
On more point community should agree on - imports ordering and grouping. I
will post another email.
--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
>