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" <dsetrak...@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 <yzhda...@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 <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?
> > > > >
> > > >
> > > > 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
> > > >
> > >
> >
signature.asc
Description: Digital signature
