ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Raul Kripalani <ra...@apache.org>
Subject Re: MQTT Streamer Code Style and Javadoc fixes (IGNITE-1747)
Date Fri, 23 Oct 2015 16:56:22 GMT
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 <yzhdanov@gridgain.com>
wrote:

> Raul,
>
> My comments are below.
>
> 2015-10-22 22:22 GMT+03:00 Raul Kripalani <raulk@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
>

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