Hello, Anton. > I'd like to propose to have only non-obvious params explained at > non-public method's Javadoc.
Non-obvious for whom? Please, remember about guys who just came into Ignite community. В Чт, 15/08/2019 в 10:05 +0300, Anton Vinogradov пишет: > My +1 to optional Javadoc at private methods, fields, and classes. > The reviewer should guaranty that everything is clear. > But, in case everything is clear without redundant Javadoc there is no need > to have it. > > So, my proposal is to allow /** */ for non-public fields and methods (to > keep readability between documented and obvious fields/methods). > Also, I'd like to propose to have only non-obvious params explained at > non-public method's Javadoc. > > On Sun, Aug 11, 2019 at 9:13 AM Павлухин Иван <vololo...@gmail.com> wrote: > > > Maxim, > > > > > I'd prefer to leave the current situation with Javadoc as it is and just > > > > to ask to apply the patch [1][2]. Can you? :-) > > > > You caught me. I left my comments for that PR > > > > .Pavel and all, > > > > > I think that API of our core internal things like PageMemory, WAL, all > > > > existing managers and processors should be as well documented as possible. > > > > No doubts here. > > > > > Documentation should be a result of a proper code review. > > > > I suppose it does not mean that documentation should be written after > > a review. I suppose it means that we should not have a poor > > documentation *after* a review. Overall, Pavel's last message conforms > > well with my opinion on the subject. > > > > чт, 8 авг. 2019 г. в 18:34, Pavel Kovalenko <jokse...@gmail.com>: > > > > > > I can agree that some part of javadocs we have is useless. It relates to > > > DTOs, getters/setters without side-effects, short self-descriptive > > > > methods. > > > In an ideal world, proper modularization of architecture, leading to > > > KISS/SOLID/DRY/etc. principles, writing self-documented code should > > > > result > > > in javadocs disappearing, as they become not needed. > > > We live in a not ideal world. We don't have good architecture and can't > > > always lead to mentioned principles, because we need sometimes sacrifice > > > readability for optimization, fixing a critical bug, etc. > > > I think that API of our core internal things like PageMemory, WAL, all > > > existing managers and processors should be as well documented as > > > > possible. > > > If a developer uses some module / manager / processor without looking > > > inside, reading the only description of public methods, it's a good sign > > > that this part of the functionality is well documented. > > > Internal implementation should be also clear for a developer who likes to > > > make a change inside it. Every optimization, avoiding race-condition, not > > > obvious thing and especially crutch must be documented as detailed as > > > possible. > > > Documentation should be a result of a proper code review. If a reviewer > > > > has > > > questions regarding any code line it should be either refactored to make > > > this thing obvious or well documented. > > > If a class or method is self-documented and obvious there is no need to > > > document it anyway. > > > if each person takes the code review as seriously as possible, > > > documentation and code will be better automatically. > > > Mandatory documentation in places where it's really not needed looks > > > > like a > > > burden. A developer will avoid write it properly everywhere or do it > > > > "just > > > for check" and this will influence on documentation with the negative > > > > side. > > > Flexible approach with mandatory / optional javadocs with good code > > > > review > > > will result in readability improvement overall. > > > > > > > > > чт, 8 авг. 2019 г. в 17:52, Maxim Muzafarov <maxmu...@gmail.com>: > > > > > > > Ivan, > > > > > > > > It is not a problem to check Javadocs at the moment code syle checking > > > > performed, but do we really need this? And the human-factor you > > > > mentioned above is also related to the `self-descriptive` names. I > > > > assume, that someone now is desiring to use single-letter variables > > > > and single-letter class names to save space an time. We will always > > > > have such an opinion race. > > > > > > > > I'd prefer to leave the current situation with Javadoc as it is and > > > > just to ask to apply the patch [1][2]. Can you? :-) > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12051 > > > > [2] https://github.com/apache/ignite/pull/6760 > > > > > > > > On Thu, 8 Aug 2019 at 17:24, Павлухин Иван <vololo...@gmail.com> > > > > wrote: > > > > > > > > > > Maxim, > > > > > > > > > > My main concern here is a human factor. Humans are usually not so > > > > good > > > > > in keeping documentation always in sync with a code. Examples from an > > > > > actual PR: > > > > > /** > > > > > * @param nodeId The remote node id. > > > > > * @param channel The channel to notify listeners with. > > > > > */ > > > > > private void onChannelOpened0(UUID nodeId, GridIoMessage initMsg, > > > > > Channel channel) > > > > > > > > > > First, there is a mismatch between number of parameters in javadoc > > > > and > > > > > code. Second, e.g. nodeId name can be made self-descriptive rmtNodeId > > > > > name. > > > > > > > > > > Mandatory javadocs do not imply good javadocs. Good javadocs do not > > > > > imply javadocs for every method and field. For me, mandatory and good > > > > > javadocs are like communism. Sounds quite nice in theory, but not > > > > > feasible in practice. > > > > > > > > > > чт, 8 авг. 2019 г. в 16:55, Denis Garus <garus....@gmail.com>: > > > > > > > > > > > > Thank you, Maxim. > > > > > > > > > > > > > > On what we are trying to save by making Javadoc optional? > > > > > > > > > > > > Space and time. > > > > > > > > > > > > I doubt that we need a verbal description of what private method > > > > make. > > > > > > Why we need it if we just can read the code? > > > > > > > > > > > > Bright examples: > > > > > > > > > > > > /** > > > > > > * @param helper Helper to attach to kernal context. > > > > > > */ > > > > > > private void addHelper(Object helper) { > > > > > > ctx.addHelper(helper); > > > > > > } > > > > > > > > > > > > /** > > > > > > * Gets "on" or "off" string for given boolean value. > > > > > > * > > > > > > * @param b Boolean value to convert. > > > > > > * @return Result string. > > > > > > */ > > > > > > private String onOff(boolean b) { > > > > > > return b ? "on" : "off"; > > > > > > } > > > > > > > > > > > > You have to support both code of method and their JavaDoc > > > > description, > > > > but > > > > > > it doesn't get any sake. > > > > > > > > > > > > > > Let's just help them to read the source code. > > > > > > > > > > > > But at the same time, public method IgniteKernal#start doesn't > > > > have any > > > > > > description except enumeration of attributes with apparent notes. > > > > > > Maybe to give it a verbal description needs one or two pages of the > > > > > > > > screen. > > > > > > Does it make sense? > > > > > > > > > > > > чт, 8 авг. 2019 г. в 15:41, Maxim Muzafarov <maxmu...@gmail.com>: > > > > > > > > > > > > > Igniters, > > > > > > > > > > > > > > I'm -1 with making Javadoc optional (except tests). > > > > > > > > > > > > > > Here is my patch [1] with PR [2] which fixes all the Javadoc > > > > comments > > > > > > > for the IgniteKernal class mentioned above. Please, take a look. > > > > The > > > > > > > review is very appreciated. > > > > > > > > > > > > > > On what we are trying to save by making Javadoc optional? In my > > > > > > > > humble > > > > > > > opinion - Javadoc comments are mostly concern users who debug the > > > > > > > Ignite when they have faced with some unhandled exception or > > > > related > > > > > > > to the community members with an average involvement (produces a > > > > few > > > > > > > small patches during the year) but not to the experienced one. > > > > Let's > > > > > > > just help them to read the source code. > > > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12051 > > > > > > > [2] https://github.com/apache/ignite/pull/6760 > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 7 Aug 2019 at 13:54, Andrey Kuznetsov <stku...@gmail.com > > > > wrote: > > > > > > > > > > > > > > > > +1 for making javadoc comments optional. > > > > > > > > > > > > > > > > - Empty and tautological comments are kind of garbage that > > > > reduce > > > > > > > > readability. > > > > > > > > - It's better to leave the entity undocumented, than write > > > > > > > > unexpressive/misleading comment. > > > > > > > > - Even classes may not require javadocs, e.g. simple DTOs. > > > > > > > > > > > > > > > > ср, 7 авг. 2019 г., 13:39 Denis Garus <garus....@gmail.com>: > > > > > > > > > > > > > > > > > Thx for feedback! > > > > > > > > > > > > > > > > > > > > we have to write proper javadoc for all production > > > > classes, > > > > > > > including > > > > > > > > > internal. > > > > > > > > > > > > > > > > > > Nikolay, I cannot agree with it. > > > > > > > > > > > > > > > > > > What should be the best comment for the next fields? > > > > > > > > > /** */ > > > > > > > > > private static final long serialVersionUID = 0L; > > > > > > > > > or > > > > > > > > > /** */ > > > > > > > > > @LoggerResource > > > > > > > > > private IgniteLogger log; > > > > > > > > > > > > > > > > > > There are more than 8000 lines of /** */ only at the > > > > ignite-core > > > > > > > module (do > > > > > > > > > not include tests). > > > > > > > > > > > > > > > > > > Any comments will be redundant and just noise. Obvious > > > > comment > > > > learns > > > > > > > > > readers skip all comments. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > identical distance/padding/margin between fields in a > > > > class - > > > > is > > > > > > > really > > > > > > > > > cool > > > > > > > > > > > > > > > > > > Vyacheslav, but we have a blank line between fields. Why is > > > > one > > > > blank > > > > > > > line > > > > > > > > > not enough? > > > > > > > > > > > > > > > > > > ср, 7 авг. 2019 г. в 12:58, Павлухин Иван < > > > > vololo...@gmail.com>: > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > Denis, thank you for starting this discussion! > > > > > > > > > > > > > > > > > > > > My opinion here is that having a good javadoc for every > > > > class > > > > and > > > > > > > > > > method is not feasible in the real world. I am quite > > > > curious > > > > to see a > > > > > > > > > > non-trivial project which follows it. Also, all comments > > > > and > > > > javadocs > > > > > > > > > > are prone to become misleading when code changes (human > > > > > > > > factor). In > > > > > > > my > > > > > > > > > > experience good method and variable names and clear code > > > > > > > > organization > > > > > > > > > > often are more helpful than javadocs. > > > > > > > > > > > > > > > > > > > > ср, 7 авг. 2019 г. в 12:49, Vyacheslav Daradur < > > > > > > > > daradu...@gmail.com > > > > > > > > : > > > > > > > > > > > > > > > > > > > > > > I agree that useless comments look weird in the codebase. > > > > > > > > > > > > > > > > > > > > > > But, identical distance/padding/margin between fields in > > > > a > > > > class - > > > > > > > is > > > > > > > > > > > really cool, and helps read the class very fast. > > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 7, 2019 at 12:26 PM Nikolay Izhikov < > > > > > > > > > > > > > > nizhi...@apache.org> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hello, Denis. > > > > > > > > > > > > > > > > > > > > > > > > Thanks for starting this discussion. > > > > > > > > > > > > > > > > > > > > > > > > I think we have to write proper javadoc for all > > > > production > > > > > > > classes, > > > > > > > > > > including internal. > > > > > > > > > > > > We should fix useless javadoc you provide. > > > > > > > > > > > > We should not accept patches without good javadocs. > > > > > > > > > > > > > > > > > > > > > > > > As for the tests, seems, we can make javadoc optional. > > > > > > > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > В Ср, 07/08/2019 в 11:41 +0300, Denis Garus пишет: > > > > > > > > > > > > > Igniters! > > > > > > > > > > > > > > > > > > > > > > > > > > I think it's time to change coding guidelines in > > > > part of > > > > > > > JavaDoc > > > > > > > > > > comments > > > > > > > > > > > > > [1]: > > > > > > > > > > > > > > > Every method, field or initializer public, > > > > private or > > > > > > > protected > > > > > > > > > > in > > > > > > > > > > > > > > > > > > > > > > > > > > top-level, > > > > > > > > > > > > > > > inner or anonymous type should have at least > > > > minimal > > > > > > > Javadoc > > > > > > > > > > comments > > > > > > > > > > > > > > > > > > > > > > > > > > including > > > > > > > > > > > > > > > description and description of parameters using > > > > > > > > @param, > > > > > > > @return > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > > > > > > > > @throws Javadoc tags, > > > > > > > > > > > > > > > where applicable. > > > > > > > > > > > > > > > > > > > > > > > > > > Let's look at JavaDoc comments in the IgniteKernal > > > > class: > > > > > > > > > > > > > > > > > > > > > > > > > > Why? > > > > > > > > > > > > > > > > > > > > > > > > > > /** */ - 15 matches. > > > > > > > > > > > > > > > > > > > > > > > > > > What can you get new from these comments? > > > > > > > > > > > > > > > > > > > > > > > > > > /** Periodic starvation check interval. */ > > > > > > > > > > > > > private static final long > > > > PERIODIC_STARVATION_CHECK_FREQ > > > > = > > > > > > > 1000 * > > > > > > > > > 30; > > > > > > > > > > > > > > > > > > > > > > > > > > /** Long jvm pause detector. */ > > > > > > > > > > > > > private LongJVMPauseDetector longJVMPauseDetector; > > > > > > > > > > > > > > > > > > > > > > > > > > /** Scheduler. */ > > > > > > > > > > > > > private IgniteScheduler scheduler; > > > > > > > > > > > > > > > > > > > > > > > > > > /** Stop guard. */ > > > > > > > > > > > > > private final AtomicBoolean stopGuard = new > > > > > > > > > > > > > > AtomicBoolean(); > > > > > > > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > > > > * @param cfg Configuration to use. > > > > > > > > > > > > > * @param utilityCachePool Utility cache pool. > > > > > > > > > > > > > * @param execSvc Executor service. > > > > > > > > > > > > > * @param sysExecSvc System executor service. > > > > > > > > > > > > > * @param stripedExecSvc Striped executor. > > > > > > > > > > > > > * @param p2pExecSvc P2P executor service. > > > > > > > > > > > > > * @param mgmtExecSvc Management executor > > > > service. > > > > > > > > > > > > > * @param igfsExecSvc IGFS executor service. > > > > > > > > > > > > > * @param dataStreamExecSvc data stream executor > > > > > > > > service. > > > > > > > > > > > > > * @param restExecSvc Reset executor service. > > > > > > > > > > > > > * @param affExecSvc Affinity executor service. > > > > > > > > > > > > > * @param idxExecSvc Indexing executor service. > > > > > > > > > > > > > * @param callbackExecSvc Callback executor > > > > service. > > > > > > > > > > > > > * @param qryExecSvc Query executor service. > > > > > > > > > > > > > * @param schemaExecSvc Schema executor service. > > > > > > > > > > > > > * @param customExecSvcs Custom named executors. > > > > > > > > > > > > > * @param errHnd Error handler to use for > > > > > > > > notification > > > > > > > about > > > > > > > > > > startup > > > > > > > > > > > > > problems. > > > > > > > > > > > > > * @param workerRegistry Worker registry. > > > > > > > > > > > > > * @param hnd Default uncaught exception handler > > > > > > > > used by > > > > > > > thread > > > > > > > > > > pools. > > > > > > > > > > > > > * @throws IgniteCheckedException Thrown in case > > > > of > > > > any > > > > > > > errors. > > > > > > > > > > > > > */ > > > > > > > > > > > > > public void start( > > > > > > > > > > > > > final IgniteConfiguration cfg, > > > > > > > > > > > > > ExecutorService utilityCachePool, > > > > > > > > > > > > > final ExecutorService execSvc, > > > > > > > > > > > > > final ExecutorService svcExecSvc, > > > > > > > > > > > > > final ExecutorService sysExecSvc, > > > > > > > > > > > > > final StripedExecutor stripedExecSvc, > > > > > > > > > > > > > ExecutorService p2pExecSvc, > > > > > > > > > > > > > ExecutorService mgmtExecSvc, > > > > > > > > > > > > > ExecutorService igfsExecSvc, > > > > > > > > > > > > > StripedExecutor dataStreamExecSvc, > > > > > > > > > > > > > ExecutorService restExecSvc, > > > > > > > > > > > > > ExecutorService affExecSvc, > > > > > > > > > > > > > @Nullable ExecutorService idxExecSvc, > > > > > > > > > > > > > IgniteStripedThreadPoolExecutor > > > > callbackExecSvc, > > > > > > > > > > > > > ExecutorService qryExecSvc, > > > > > > > > > > > > > ExecutorService schemaExecSvc, > > > > > > > > > > > > > @Nullable final Map<String, ? extends > > > > > > > > ExecutorService> > > > > > > > > > > > > > customExecSvcs, > > > > > > > > > > > > > GridAbsClosure errHnd, > > > > > > > > > > > > > WorkersRegistry workerRegistry, > > > > > > > > > > > > > Thread.UncaughtExceptionHandler hnd, > > > > > > > > > > > > > TimeBag startTimer > > > > > > > > > > > > > ) > > > > > > > > > > > > > > > > > > > > > > > > > > These comments look ugly and useless, and that is the > > > > > > > > main > > > > > > > class of > > > > > > > > > > core. > > > > > > > > > > > > > Why do they need us? > > > > > > > > > > > > > Let us change the coding guidelines in part of > > > > JavaDoc > > > > > > > comments: > > > > > > > > > > > > > Every method public API should have at least minimal > > > > > > > > Javadoc > > > > > > > > > > comments, > > > > > > > > > > > > > including description and description of parameters > > > > using > > > > > > > @param, > > > > > > > > > > @return, > > > > > > > > > > > > > and @throws Javadoc tags, > > > > > > > > > > > > > where applicable. > > > > > > > > > > > > > For internal API, JavaDoc comments should be > > > > optional. > > > > It's up > > > > > > > to a > > > > > > > > > > > > > contributor or reviewer. > > > > > > > > > > > > > > > > > > > > > > > > > > What are you think? > > > > > > > > > > > > > > > > > > > > > > > > > > 1. > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-JavadocComments > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Best regards, > > > > > > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > -- > > Best regards, > > Ivan Pavlukhin > >
signature.asc
Description: This is a digitally signed message part
