Sounds reasonable. I've satrted Data Structures suite hang investigation [1].
Igniters, especially commiters, I know, you're busy, but it will be a great help to the project in case you fix at least one hang per person. [1] https://issues.apache.org/jira/browse/IGNITE-8783 пн, 9 июл. 2018 г. в 19:24, Maxim Muzafarov <maxmu...@gmail.com>: > Hi Igniters, > > Let's back to discussion of review checklist. Can we add more clarification > about running all suites on TeamCity? > > My suggestion is: “All test suites MUST be run on TeamCity [3] before merge > to master, there MUST NOT be any test failures * and any tests\suites with > “execution timeouts” *. Not important test failures should be muted and > handled according to [4] process.” > > As you can see we have stable “Execution timeouts” for > “Activate\Deactiveate Cluster” test suite since 16-th June. How can we be > sure in this case that new changes would not break up old functionality? > > From my point, all new changes MUST NOT be merged to master util we will > fix all execution timeouts for suites. Even if current changes are not > related to these timeouts. > > [1] > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ActivateDeactivateCluster&tab=buildTypeHistoryList&branch_IgniteTests24Java8=%3Cdefault%3E > > > пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov <dpavlov....@gmail.com>: > > > Requirement of green TC for each PR is community rule, not my. > > > > I'll answer ro another question, what should we do with test failure: > > "Ideally - fix, but at least mute test and create ticket. " > > > > May be it's time to formalize Make TC Green Again process in details, so > > let me share my draft. > > > > If Igniter see test failure (in master, in release bracnh, etc), he > should > > consider following steps: > > > > - If your changes can led to this failure(s), please create issue with > > label MakeTeamCityGreenAgain and assign it to you. > > - If you have fix, please set ticket to PA state and write to dev > > list fix is ready. > > - For case fix will require some time please mute test and set > label > > Muted_Test to issue > > - If you know which change caused failure please contact change author > > directly. > > - If you don't know which change caused failure please send message to > > dev list to find out > > > > > > > > > > пн, 4 июн. 2018 г. в 15:27, Vladimir Ozerov <voze...@gridgain.com>: > > > > > Dmitry, > > > > > > My question was how to proceed with your rules. Could you please > clarify? > > > > > > On Mon, Jun 4, 2018 at 2:52 PM, Dmitry Pavlov <dpavlov....@gmail.com> > > > wrote: > > > > > > > Vladimir, I mean strict definition, how much previous runs should > > > > contributor consider? What if test was failed by infrastructure > reason > > in > > > > master previously, how can contributor be sure test failure != broken > > > code > > > > in PR? In this case it should be double checked by > > contributor/reviewer. > > > > I'm sure nobody can give strict definition of 'new' failure. > > > > > > > > Flaky tests detected by TC may be taken into account in check-list, > > > because > > > > contributor can check if failure is flaky. But again, not all tests > > with > > > > floating failure is detected by TC as flaky. > > > > > > > > I don't understand what problem will be solved if we soften current > > > > requirement with 'new' test? Everybody will continue to complain they > > > PR's > > > > test failures is not `new`. So let's keep it as is. > > > > > > > > пн, 4 июн. 2018 г. в 14:46, Vladimir Ozerov <voze...@gridgain.com>: > > > > > > > > > Dmitry, > > > > > > > > > > New failure is a failure hasn't happened on previous runs. If it do > > > > > happened, then contributor should see if it is a flaky or not > through > > > > local > > > > > and TC runs. The same works for timeout suites. > > > > > Current statement in "Review Checklist" that there are should be no > > > > failed > > > > > tests is not applicable to real word. Almost every patch is pushed > to > > > > > repository with test failures. > > > > > > > > > > On Mon, Jun 4, 2018 at 2:22 PM, Dmitry Pavlov < > dpavlov....@gmail.com > > > > > > > > wrote: > > > > > > > > > > > Hi Vladimir, could you provide definition what is new failure? > how > > do > > > > you > > > > > > know it is new or not? > > > > > > > > > > > > And please forget for a moment you're Ignite expert & veteran, > > > imagine > > > > > you > > > > > > are newcomer. > > > > > > > > > > > > I can't find any criteria that can be used by newbie to come to > the > > > > > > conclusion that test is new. Patch is accepted by reviewer, so it > > > > should > > > > > be > > > > > > up to him to correctly register failures in tickets with > > > > > > MakeTeamCityGreenAgain label and mute unimportant tests. > > > > > > > > > > > > пн, 4 июн. 2018 г. в 11:32, Vladimir Ozerov < > voze...@gridgain.com > > >: > > > > > > > > > > > > > Dmitry, > > > > > > > > > > > > > > I still do not see how new patches could be accepted with this > > > > > > requirement > > > > > > > in place. Consider the following case: I created a patch and > run > > it > > > > on > > > > > > TC, > > > > > > > observed N failures, verified through TC history that none if > > them > > > > are > > > > > > new. > > > > > > > Am I eligible to push the commit? > > > > > > > > > > > > > > On Thu, May 24, 2018 at 3:11 PM, Dmitry Pavlov < > > > > dpavlov....@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Petr, good point. It is more intuitive, we should mark test > we > > > can > > > > > > ignore > > > > > > > > by mute. > > > > > > > > > > > > > > > > So Vladimir, you or other Ignite veteran can mute test, if > can > > > say > > > > it > > > > > > is > > > > > > > > not important. > > > > > > > > > > > > > > > > чт, 24 мая 2018 г. в 15:07, Petr Ivanov <mr.wei...@gmail.com > >: > > > > > > > > > > > > > > > > > Why cannot we mute (and file corresponding tickets) all > test > > > > > failures > > > > > > > > > (including flaky) to some date and start initiative Green > TC? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 24 May 2018, at 15:04, Vladimir Ozerov < > > > > voze...@gridgain.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Dmitry, > > > > > > > > > > > > > > > > > > > > We cannot add this requirements, because we do have > > failures > > > on > > > > > TC. > > > > > > > > This > > > > > > > > > > requirement implies that all development would stop until > > TC > > > is > > > > > > > green. > > > > > > > > > > We never had old requirement work, neither we need to > > enforce > > > > it > > > > > > now. > > > > > > > > > > > > > > > > > > > > On Thu, May 24, 2018 at 2:59 PM, Dmitry Pavlov < > > > > > > > dpavlov....@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > >> 3.c > > > > > > > > > >> > > > > > > > > > >> 1. All test suites *MUST* be run on TeamCity [3] > before > > > > merge > > > > > to > > > > > > > > > master, > > > > > > > > > >> there *MUST NOT* be any test failures > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> 'New' word should be removed because we cant separate > > `new` > > > > and > > > > > > `non > > > > > > > > > new` > > > > > > > > > >> failures. > > > > > > > > > >> > > > > > > > > > >> Let's imagine example, we have 50 green runs in master. > > And > > > PR > > > > > > > Run-All > > > > > > > > > >> contains this test failed. Is it new or not new? > Actually > > we > > > > > don't > > > > > > > > know. > > > > > > > > > >> > > > > > > > > > >> Existing requirement is about all TC must be green, so > > let's > > > > > keep > > > > > > it > > > > > > > > as > > > > > > > > > is. > > > > > > > > > >> > > > > > > > > > >> ср, 23 мая 2018 г. в 17:02, Vladimir Ozerov < > > > > > voze...@gridgain.com > > > > > > >: > > > > > > > > > >> > > > > > > > > > >>> Igniters, > > > > > > > > > >>> > > > > > > > > > >>> I created review checklist on WIKI [1] and also fixed > > > related > > > > > > pages > > > > > > > > > (e.g. > > > > > > > > > >>> "How To Contribute"). Please let me know if you have > any > > > > > comments > > > > > > > > > before > > > > > > > > > >> I > > > > > > > > > >>> go with public announce. > > > > > > > > > >>> > > > > > > > > > >>> Vladimir. > > > > > > > > > >>> > > > > > > > > > >>> [1] > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist > > > > > > > > > >>> > > > > > > > > > >>> On Thu, May 10, 2018 at 5:10 PM, Vladimir Ozerov < > > > > > > > > voze...@gridgain.com > > > > > > > > > > > > > > > > > > > >>> wrote: > > > > > > > > > >>> > > > > > > > > > >>>> Ilya, > > > > > > > > > >>>> > > > > > > > > > >>>> We define that exception messages *SHOULD* have clear > > > > > > explanation > > > > > > > on > > > > > > > > > >> what > > > > > > > > > >>>> is wrong. *SHOULD* mean that the rule should be > followed > > > > > unless > > > > > > > > there > > > > > > > > > >> is > > > > > > > > > >>> a > > > > > > > > > >>>> reason not to follow. In your case you refer to some > > > > > unexpected > > > > > > > > > >> behavior. > > > > > > > > > >>>> I.e. an exceptional situation developer is not aware > of. > > > In > > > > > this > > > > > > > > case > > > > > > > > > >> for > > > > > > > > > >>>> sure we cannot force contributor to explain what is > > wrong, > > > > > > > because, > > > > > > > > > >> well, > > > > > > > > > >>>> we don't know. This is why we relaxed the rule from > > *MUST* > > > > to > > > > > > > > > *SHOULD*. > > > > > > > > > >>>> > > > > > > > > > >>>> On Thu, May 10, 2018 at 3:50 PM, Ilya Kasnacheev < > > > > > > > > > >>>> ilya.kasnach...@gmail.com> wrote: > > > > > > > > > >>>> > > > > > > > > > >>>>> I don't think I quite understand how exception > > > explanations > > > > > > > should > > > > > > > > > >> work. > > > > > > > > > >>>>> > > > > > > > > > >>>>> Imagine we have the following exception: > > > > > > > > > >>>>> > > > > > > > > > >>>>> // At least RuntimeException can be thrown by the > code > > > > above > > > > > > when > > > > > > > > > >>>>> GridCacheContext is cleaned and there is > > > > > > > > > >>>>> // an attempt to use cleaned resources. > > > > > > > > > >>>>> U.error(log, "Unexpected exception during cache > > update", > > > > e); > > > > > > > > > >>>>> > > > > > > > > > >>>>> I mean, we genuinely don't know what happened here. > > > > > > > > > >>>>> > > > > > > > > > >>>>> Under new rules, what kind of "workaround" would that > > > > > exception > > > > > > > > > >> suggest? > > > > > > > > > >>>>> "Try turning it off and then back on"? > > > > > > > > > >>>>> What explanation how to resolve this exception can we > > > > offer? > > > > > > > > "Please > > > > > > > > > >>> write > > > > > > > > > >>>>> to d...@apache.ignite.org or to Apache JIRA, and then > > > wait > > > > > for > > > > > > a > > > > > > > > > >> release > > > > > > > > > >>>>> with fix?" > > > > > > > > > >>>>> > > > > > > > > > >>>>> I'm really confused how we can implement 1.6 and 1.7 > > when > > > > > > dealing > > > > > > > > > with > > > > > > > > > >>>>> messy real-world code. > > > > > > > > > >>>>> > > > > > > > > > >>>>> Regards, > > > > > > > > > >>>>> > > > > > > > > > >>>>> > > > > > > > > > >>>>> -- > > > > > > > > > >>>>> Ilya Kasnacheev > > > > > > > > > >>>>> > > > > > > > > > >>>>> 2018-05-10 11:39 GMT+03:00 Vladimir Ozerov < > > > > > > voze...@gridgain.com > > > > > > > >: > > > > > > > > > >>>>> > > > > > > > > > >>>>>> Andrey, Anton, Alex > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> Agree, *SHOULD* is more appropriate here. > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> Please see latest version below. Does anyone want to > > add > > > > or > > > > > > > change > > > > > > > > > >>>>>> something? Let's wait for several days for more > > feedback > > > > and > > > > > > > then > > > > > > > > > >>>>> publish > > > > > > > > > >>>>>> and announce this list. Note that it would not be > > carved > > > > in > > > > > > > stone > > > > > > > > > >> and > > > > > > > > > >>> we > > > > > > > > > >>>>>> will be able to change it at any time if needed. > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> 1) API > > > > > > > > > >>>>>> 1.1) API compatibility *MUST* be maintained between > > > minor > > > > > > > > releases. > > > > > > > > > >> Do > > > > > > > > > >>>>> not > > > > > > > > > >>>>>> remove existing methods or change their signatures, > > > > > deprecate > > > > > > > them > > > > > > > > > >>>>> instead > > > > > > > > > >>>>>> 1.2) Default behaviour "SHOULD NOT* be changed > between > > > > minor > > > > > > > > > >> releases, > > > > > > > > > >>>>>> unless absolutely needed. If change is made, it > *MUST* > > > be > > > > > > > > described > > > > > > > > > >> in > > > > > > > > > >>>>>> "Migration Guide" > > > > > > > > > >>>>>> 1.3) New operation *MUST* be well-documented in code > > > > > (javadoc, > > > > > > > > > >>>>> dotnetdoc): > > > > > > > > > >>>>>> documentation must contain method's purpose, > > description > > > > of > > > > > > > > > >> parameters > > > > > > > > > >>>>> and > > > > > > > > > >>>>>> how their values affect the outcome, description of > > > return > > > > > > value > > > > > > > > and > > > > > > > > > >>>>> it's > > > > > > > > > >>>>>> default, behavior in negative cases, interaction > with > > > > other > > > > > > > > > >> operations > > > > > > > > > >>>>> and > > > > > > > > > >>>>>> components > > > > > > > > > >>>>>> 1.4) API parity between Java and .NET platforms > > *SHOULD* > > > > be > > > > > > > > > >> maintained > > > > > > > > > >>>>> when > > > > > > > > > >>>>>> operation makes sense on both platforms. If method > > > cannot > > > > be > > > > > > > > > >>>>> implemented in > > > > > > > > > >>>>>> a platform immediately, new JIRA ticket *MUST* be > > > created > > > > > and > > > > > > > > linked > > > > > > > > > >>> to > > > > > > > > > >>>>>> current ticket > > > > > > > > > >>>>>> 1.5) API parity between thin clients (Java, .NET) > > > *SHOULD* > > > > > be > > > > > > > > > >>> maintained > > > > > > > > > >>>>>> when operation makes sense on several clients. If > > method > > > > > > cannot > > > > > > > be > > > > > > > > > >>>>>> implemented in a client immediately, new JIRA ticket > > > > *MUST* > > > > > be > > > > > > > > > >> created > > > > > > > > > >>>>> and > > > > > > > > > >>>>>> linked to current ticket > > > > > > > > > >>>>>> 1.6) All exceptions thrown to a user **SHOULD** have > > > > > > explanation > > > > > > > > how > > > > > > > > > >>> to > > > > > > > > > >>>>>> resolve, workaround or debug an error > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> 2) Compatibility > > > > > > > > > >>>>>> 2.1) Persistence backward compatibility *MUST* be > > > > maintained > > > > > > > > between > > > > > > > > > >>>>> minor > > > > > > > > > >>>>>> releases. It should be possible to start newer > version > > > on > > > > > data > > > > > > > > files > > > > > > > > > >>>>>> created by the previous version > > > > > > > > > >>>>>> 2.2) Thin client forward and backward compatibility > > > > *SHOULD* > > > > > > be > > > > > > > > > >>>>> maintained > > > > > > > > > >>>>>> between two consecutive minor releases. If > > compatibility > > > > > > cannot > > > > > > > be > > > > > > > > > >>>>>> maintained it *MUST* be described in "Migration > Guide" > > > > > > > > > >>>>>> 2.3) JDBC and ODBC forward and backward > compatibility > > > > > *SHOULD* > > > > > > > be > > > > > > > > > >>>>>> maintained between two consecutive minor releases. > If > > > > > > > > compatibility > > > > > > > > > >>>>> cannot > > > > > > > > > >>>>>> be maintained it *MUST* be described in "Migration > > > Guide" > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> 3) Tests > > > > > > > > > >>>>>> 3.1) New functionality *MUST* be covered with unit > > tests > > > > for > > > > > > > both > > > > > > > > > >>>>> positive > > > > > > > > > >>>>>> and negative use cases > > > > > > > > > >>>>>> 3.2) All test suites *MUST* be run before merge to > > > > > > master..There > > > > > > > > > >>> *MUST* > > > > > > > > > >>>>> be > > > > > > > > > >>>>>> no new test failures > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> 4) Code style *MUST* be followed as per Ignite's > > Coding > > > > > > > Guidelines > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> Vladimir. > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> > > > > > > > > > >>>>>> On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov < > > > > > > > > stku...@gmail.com > > > > > > > > > >>> > > > > > > > > > >>>>>> wrote: > > > > > > > > > >>>>>> > > > > > > > > > >>>>>>> Anton, > > > > > > > > > >>>>>>> > > > > > > > > > >>>>>>> I agree, *MUST* for exception reasons and *SHOULD* > > for > > > > ways > > > > > > of > > > > > > > > > >>>>> resolution > > > > > > > > > >>>>>>> sound clearer. > > > > > > > > > >>>>>>> > > > > > > > > > >>>>>>> 2018-05-08 12:56 GMT+03:00 Anton Vinogradov < > > > > a...@apache.org > > > > > >: > > > > > > > > > >>>>>>> > > > > > > > > > >>>>>>>> Andrey, > > > > > > > > > >>>>>>>> > > > > > > > > > >>>>>>>> How about > > > > > > > > > >>>>>>>> 1.6) All exceptions thrown to a user *MUST* have > > > > > explanation > > > > > > > of > > > > > > > > > >>>>>>> workaround > > > > > > > > > >>>>>>>> and contain original error. > > > > > > > > > >>>>>>>> All exceptions thrown to a user *SHOULD* have > > > > explanation > > > > > > how > > > > > > > to > > > > > > > > > >>>>>> resolve > > > > > > > > > >>>>>>> if > > > > > > > > > >>>>>>>> possible. > > > > > > > > > >>>>>>>> ? > > > > > > > > > >>>>>>>> > > > > > > > > > >>>>>>>> вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov < > > > > > > > stku...@gmail.com > > > > > > > > > >>> : > > > > > > > > > >>>>>>>> > > > > > > > > > >>>>>>>>> Vladimir, checklist looks pleasant enough for me. > > > > > > > > > >>>>>>>>> > > > > > > > > > >>>>>>>>> I'd like to suggest one minor change. In 1.6 > *MUST* > > > > seems > > > > > > to > > > > > > > > > >> be > > > > > > > > > >>>>> too > > > > > > > > > >>>>>>>> strict, > > > > > > > > > >>>>>>>>> *SHOULD* would be enough. It can be frustrating > for > > > API > > > > > > user > > > > > > > > > >> if > > > > > > > > > >>> I > > > > > > > > > >>>>>>> explain > > > > > > > > > >>>>>>>>> how to fix NPEs in a trivial way, for example. > > > > > > > > > >>>>>>>>> > > > > > > > > > >>>>>>>>> 2018-05-08 11:34 GMT+03:00 Anton Vinogradov < > > > > > a...@apache.org > > > > > > >: > > > > > > > > > >>>>>>>>> > > > > > > > > > >>>>>>>>>> Alex, > > > > > > > > > >>>>>>>>>> > > > > > > > > > >>>>>>>>>> It is not sounds like that, obviously. > > > > > > > > > >>>>>>>>>> > > > > > > > > > >>>>>>>>>> Tests should cover all negative and positive > > cases. > > > > > > > > > >>>>>>>>>> You should add enough tests to cover all cases. > > > > > > > > > >>>>>>>>>> > > > > > > > > > >>>>>>>>>> Sometimes one test can cover more than one case, > > so > > > > two > > > > > > > > > >> tests > > > > > > > > > >>>>> *CAN* > > > > > > > > > >>>>>>>>>> partially check same things. > > > > > > > > > >>>>>>>>>> In case some cases already covered you should > not > > > > create > > > > > > > > > >>>>>> duplicates. > > > > > > > > > >>>>>>>>>> > > > > > > > > > >>>>>>>>>> вт, 8 мая 2018 г. в 10:19, Александр Меньшиков < > > > > > > > > > >>>>>> sharple...@gmail.com > > > > > > > > > >>>>>>>> : > > > > > > > > > >>>>>>>>>> > > > > > > > > > >>>>>>>>>>> Vladimir, the 3.1 is a bit unclear for me. > Which > > > code > > > > > > > > > >>>>> coverage is > > > > > > > > > >>>>>>>>>>> acceptable? Now it sounds like two tests are > > enough > > > > > (one > > > > > > > > > >> for > > > > > > > > > >>>>>>> positive > > > > > > > > > >>>>>>>>> and > > > > > > > > > >>>>>>>>>>> one for negative cases). > > > > > > > > > >>>>>>>>>>> > > > > > > > > > >>>>>>>>>>> 2018-05-07 23:09 GMT+03:00 Dmitriy Setrakyan < > > > > > > > > > >>>>>>> dsetrak...@apache.org > > > > > > > > > >>>>>>>>> : > > > > > > > > > >>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>> Is this list on the Wiki? > > > > > > > > > >>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>> On Mon, May 7, 2018 at 7:26 AM, Vladimir > Ozerov > > < > > > > > > > > > >>>>>>>>> voze...@gridgain.com> > > > > > > > > > >>>>>>>>>>>> wrote: > > > > > > > > > >>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> Igniters, > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> This is the checklist I have at the moment. > > > Please > > > > > let > > > > > > > > > >>> me > > > > > > > > > >>>>>> know > > > > > > > > > >>>>>>> if > > > > > > > > > >>>>>>>>> you > > > > > > > > > >>>>>>>>>>>> have > > > > > > > > > >>>>>>>>>>>>> any comments on existing items, or want to > add > > or > > > > > > > > > >> remove > > > > > > > > > >>>>>>>> anything. > > > > > > > > > >>>>>>>>> It > > > > > > > > > >>>>>>>>>>>> looks > > > > > > > > > >>>>>>>>>>>>> like we may have not only strict rules, but > > *nice > > > > to > > > > > > > > > >>> have* > > > > > > > > > >>>>>>> points > > > > > > > > > >>>>>>>>>> here > > > > > > > > > >>>>>>>>>>> as > > > > > > > > > >>>>>>>>>>>>> well with help of *MUST*, *SHOULD* and *MAY* > > > words > > > > as > > > > > > > > > >>> per > > > > > > > > > >>>>>>> RFC2119 > > > > > > > > > >>>>>>>>>> [1]. > > > > > > > > > >>>>>>>>>>> So > > > > > > > > > >>>>>>>>>>>>> please feel free to suggest optional items as > > > well. > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> 1) API > > > > > > > > > >>>>>>>>>>>>> 1.1) API compatibility *MUST* be maintained > > > between > > > > > > > > > >>> minor > > > > > > > > > >>>>>>>> releases. > > > > > > > > > >>>>>>>>>> Do > > > > > > > > > >>>>>>>>>>>> not > > > > > > > > > >>>>>>>>>>>>> remove existing methods or change their > > > signatures, > > > > > > > > > >>>>> deprecate > > > > > > > > > >>>>>>>> them > > > > > > > > > >>>>>>>>>>>> instead > > > > > > > > > >>>>>>>>>>>>> 1.2) Default behaviour "SHOULD NOT* be > changed > > > > > between > > > > > > > > > >>>>> minor > > > > > > > > > >>>>>>>>>> releases, > > > > > > > > > >>>>>>>>>>>>> unless absolutely needed. If change is made, > it > > > > > *MUST* > > > > > > > > > >>> be > > > > > > > > > >>>>>>>> described > > > > > > > > > >>>>>>>>>> in > > > > > > > > > >>>>>>>>>>>>> "Migration Guide" > > > > > > > > > >>>>>>>>>>>>> 1.3) New operation *MUST* be well-documented > in > > > > code > > > > > > > > > >>>>>> (javadoc, > > > > > > > > > >>>>>>>>>>>> dotnetdoc): > > > > > > > > > >>>>>>>>>>>>> documentation must contain method's purpose, > > > > > > > > > >> description > > > > > > > > > >>>>> of > > > > > > > > > >>>>>>>>>> parameters > > > > > > > > > >>>>>>>>>>>> and > > > > > > > > > >>>>>>>>>>>>> how their values affect the outcome, > > description > > > of > > > > > > > > > >>> return > > > > > > > > > >>>>>>> value > > > > > > > > > >>>>>>>>> and > > > > > > > > > >>>>>>>>>>> it's > > > > > > > > > >>>>>>>>>>>>> default, behavior in negative cases, > > interaction > > > > with > > > > > > > > > >>>>> other > > > > > > > > > >>>>>>>>>> operations > > > > > > > > > >>>>>>>>>>>> and > > > > > > > > > >>>>>>>>>>>>> components > > > > > > > > > >>>>>>>>>>>>> 1.4) API parity between Java and .NET > platforms > > > > > > > > > >> *SHOULD* > > > > > > > > > >>>>> be > > > > > > > > > >>>>>>>>>> maintained > > > > > > > > > >>>>>>>>>>>> when > > > > > > > > > >>>>>>>>>>>>> operation makes sense on both platforms. If > > > method > > > > > > > > > >>> cannot > > > > > > > > > >>>>> be > > > > > > > > > >>>>>>>>>>> implemented > > > > > > > > > >>>>>>>>>>>> in > > > > > > > > > >>>>>>>>>>>>> a platform immediately, new JIRA ticket > *MUST* > > be > > > > > > > > > >>> created > > > > > > > > > >>>>> and > > > > > > > > > >>>>>>>>> linked > > > > > > > > > >>>>>>>>>> to > > > > > > > > > >>>>>>>>>>>>> current ticket > > > > > > > > > >>>>>>>>>>>>> 1.5) API parity between thin clients (Java, > > .NET) > > > > > > > > > >>>>> *SHOULD* be > > > > > > > > > >>>>>>>>>>> maintained > > > > > > > > > >>>>>>>>>>>>> when operation makes sense on several > clients. > > If > > > > > > > > > >> method > > > > > > > > > >>>>>> cannot > > > > > > > > > >>>>>>>> be > > > > > > > > > >>>>>>>>>>>>> implemented in a client immediately, new JIRA > > > > ticket > > > > > > > > > >>>>> *MUST* > > > > > > > > > >>>>>> be > > > > > > > > > >>>>>>>>>> created > > > > > > > > > >>>>>>>>>>>> and > > > > > > > > > >>>>>>>>>>>>> linked to current ticket > > > > > > > > > >>>>>>>>>>>>> 1.6) All exceptions thrown to a user *MUST* > > have > > > > > > > > > >>>>> explanation > > > > > > > > > >>>>>>> how > > > > > > > > > >>>>>>>> to > > > > > > > > > >>>>>>>>>>>>> resolve, workaround or debug an error > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> 2) Compatibility > > > > > > > > > >>>>>>>>>>>>> 2.1) Persistence backward compatibility > *MUST* > > be > > > > > > > > > >>>>> maintained > > > > > > > > > >>>>>>>>> between > > > > > > > > > >>>>>>>>>>>> minor > > > > > > > > > >>>>>>>>>>>>> releases. It should be possible to start > newer > > > > > version > > > > > > > > > >>> on > > > > > > > > > >>>>>> data > > > > > > > > > >>>>>>>>> files > > > > > > > > > >>>>>>>>>>>>> created by the previous version > > > > > > > > > >>>>>>>>>>>>> 2.2) Thin client forward and backward > > > compatibility > > > > > > > > > >>>>> *SHOULD* > > > > > > > > > >>>>>> be > > > > > > > > > >>>>>>>>>>>> maintained > > > > > > > > > >>>>>>>>>>>>> between two consecutive minor releases. If > > > > > > > > > >> compatibility > > > > > > > > > >>>>>> cannot > > > > > > > > > >>>>>>>> be > > > > > > > > > >>>>>>>>>>>>> maintained it *MUST* be described in > "Migration > > > > > Guide" > > > > > > > > > >>>>>>>>>>>>> 2.3) JDBC and ODBC forward and backward > > > > compatibility > > > > > > > > > >>>>>> *SHOULD* > > > > > > > > > >>>>>>> be > > > > > > > > > >>>>>>>>>>>>> maintained between two consecutive minor > > > releases. > > > > If > > > > > > > > > >>>>>>>> compatibility > > > > > > > > > >>>>>>>>>>>> cannot > > > > > > > > > >>>>>>>>>>>>> be maintained it *MUST* be described in > > > "Migration > > > > > > > > > >>> Guide" > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> 3) Tests > > > > > > > > > >>>>>>>>>>>>> 3.1) New functionality *MUST* be covered with > > > unit > > > > > > > > > >> tests > > > > > > > > > >>>>> for > > > > > > > > > >>>>>>> both > > > > > > > > > >>>>>>>>>>>> positive > > > > > > > > > >>>>>>>>>>>>> and negative use cases > > > > > > > > > >>>>>>>>>>>>> 3.2) All test suites *MUST* be run before > merge > > > to > > > > > > > > > >>>>>>> master..There > > > > > > > > > >>>>>>>>>> *MUST* > > > > > > > > > >>>>>>>>>>>> be > > > > > > > > > >>>>>>>>>>>>> no new test failures > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> 4) Code style *MUST* be followed as per > > Ignite's > > > > > > > > > >> Coding > > > > > > > > > >>>>>>>> Guidelines > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> Vladimir. > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> [1] https://www.ietf.org/rfc/rfc2119.txt > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>> On Fri, May 4, 2018 at 4:33 PM, Vladimir > > Ozerov < > > > > > > > > > >>>>>>>>>> voze...@gridgain.com> > > > > > > > > > >>>>>>>>>>>>> wrote: > > > > > > > > > >>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>> Hi Dmitry, > > > > > > > > > >>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>> Yes, I'll do that in the nearest days. > > > > > > > > > >>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>> On Wed, Apr 25, 2018 at 8:24 PM, Dmitry > > Pavlov < > > > > > > > > > >>>>>>>>>>> dpavlov....@gmail.com> > > > > > > > > > >>>>>>>>>>>>>> wrote: > > > > > > > > > >>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>> Igniters, the idea was related to small > > > > > > > > > >> refactorings > > > > > > > > > >>>>>>>> co-located > > > > > > > > > >>>>>>>>>> with > > > > > > > > > >>>>>>>>>>>>> main > > > > > > > > > >>>>>>>>>>>>>>> change. > > > > > > > > > >>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>> Main change itself indicates that existing > > code > > > > did > > > > > > > > > >>> not > > > > > > > > > >>>>>> meet > > > > > > > > > >>>>>>>> the > > > > > > > > > >>>>>>>>>>>>> criteria > > > > > > > > > >>>>>>>>>>>>>>> of practice. Approving of standalone > > > refactorings > > > > > > > > > >>>>> instead > > > > > > > > > >>>>>>>>>>> contradicts > > > > > > > > > >>>>>>>>>>>>> with > > > > > > > > > >>>>>>>>>>>>>>> principle don't touch if it works. So I > still > > > > like > > > > > > > > > >>>>> idea of > > > > > > > > > >>>>>>>>>>> co-located > > > > > > > > > >>>>>>>>>>>>>>> changes improving code, javadocs, style, > etc. > > > > > > > > > >>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>> But let's not argue about this point now, > > let's > > > > > > > > > >>>>> summarize > > > > > > > > > >>>>>>> the > > > > > > > > > >>>>>>>>>>>> undisputed > > > > > > > > > >>>>>>>>>>>>>>> points and add it to the wiki. Vladimir, > > would > > > > you > > > > > > > > > >>>>> please > > > > > > > > > >>>>>> do > > > > > > > > > >>>>>>>> it? > > > > > > > > > >>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>> ср, 25 апр. 2018 г. в 16:42, Nikolay > Izhikov > > < > > > > > > > > > >>>>>>>>> nizhi...@apache.org > > > > > > > > > >>>>>>>>>>> : > > > > > > > > > >>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>> Igniters, > > > > > > > > > >>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>> I agree with Vova. > > > > > > > > > >>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>> Don't fix if it works! > > > > > > > > > >>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>> If you 100% sure then it a useful addition > > to > > > > the > > > > > > > > > >>>>>> product > > > > > > > > > >>>>>>> - > > > > > > > > > >>>>>>>>> just > > > > > > > > > >>>>>>>>>>>> make > > > > > > > > > >>>>>>>>>>>>> a > > > > > > > > > >>>>>>>>>>>>>>>> separate ticket. > > > > > > > > > >>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>> В Ср, 25/04/2018 в 11:44 +0300, Vladimir > > > Ozerov > > > > > > > > > >>>>> пишет: > > > > > > > > > >>>>>>>>>>>>>>>>> Guys, > > > > > > > > > >>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>> The problem with in-place refactorings is > > > that > > > > > > > > > >>> you > > > > > > > > > >>>>>>>> increase > > > > > > > > > >>>>>>>>>>>> affected > > > > > > > > > >>>>>>>>>>>>>>>> scope. > > > > > > > > > >>>>>>>>>>>>>>>>> It is not uncommon to break compatibility > > or > > > > > > > > > >>> public > > > > > > > > > >>>>>>>>> contracts > > > > > > > > > >>>>>>>>>>> with > > > > > > > > > >>>>>>>>>>>>>>> even > > > > > > > > > >>>>>>>>>>>>>>>>> minor things. E.g. recently we decided > drop > > > > > > > > > >>>>> org.jsr166 > > > > > > > > > >>>>>>>>> package > > > > > > > > > >>>>>>>>>>> in > > > > > > > > > >>>>>>>>>>>>>>> favor > > > > > > > > > >>>>>>>>>>>>>>>> of > > > > > > > > > >>>>>>>>>>>>>>>>> Java 8 classes. Innocent change. Result - > > > > > > > > > >> broken > > > > > > > > > >>>>>>> storage. > > > > > > > > > >>>>>>>>>>> Another > > > > > > > > > >>>>>>>>>>>>>>> problem > > > > > > > > > >>>>>>>>>>>>>>>>> is conflicts. It is not uncommon to have > > > > > > > > > >>> long-lived > > > > > > > > > >>>>>>>> branches > > > > > > > > > >>>>>>>>>>> which > > > > > > > > > >>>>>>>>>>>>> we > > > > > > > > > >>>>>>>>>>>>>>>> need > > > > > > > > > >>>>>>>>>>>>>>>>> to merge with master over and over again. > > > And a > > > > > > > > > >>>>> lot of > > > > > > > > > >>>>>>>>>>>> refactorings > > > > > > > > > >>>>>>>>>>>>>>> cause > > > > > > > > > >>>>>>>>>>>>>>>>> conflicts. It is much easier to resolve > > them > > > if > > > > > > > > > >>> you > > > > > > > > > >>>>>> know > > > > > > > > > >>>>>>>>> that > > > > > > > > > >>>>>>>>>>>> logic > > > > > > > > > >>>>>>>>>>>>>>> was > > > > > > > > > >>>>>>>>>>>>>>>> not > > > > > > > > > >>>>>>>>>>>>>>>>> affected as opposed to cases when you > need > > to > > > > > > > > > >>>>> resolve > > > > > > > > > >>>>>>> both > > > > > > > > > >>>>>>>>>>> renames > > > > > > > > > >>>>>>>>>>>>> and > > > > > > > > > >>>>>>>>>>>>>>>>> method extractions along with > > business-logic > > > > > > > > > >>>>> changes. > > > > > > > > > >>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>> I'd like to repeat - if you have a time > for > > > > > > > > > >>>>>> refactoring > > > > > > > > > >>>>>>>> then > > > > > > > > > >>>>>>>>>> you > > > > > > > > > >>>>>>>>>>>>>>>> definitely > > > > > > > > > >>>>>>>>>>>>>>>>> have a time to extract these changes to > > > > > > > > > >> separate > > > > > > > > > >>> PR > > > > > > > > > >>>>>> and > > > > > > > > > >>>>>>>>>> submit a > > > > > > > > > >>>>>>>>>>>>>>> separate > > > > > > > > > >>>>>>>>>>>>>>>>> ticket. I am quite understand what "low > > > > > > > > > >> priority" > > > > > > > > > >>>>> do > > > > > > > > > >>>>>> you > > > > > > > > > >>>>>>>>> mean > > > > > > > > > >>>>>>>>>> if > > > > > > > > > >>>>>>>>>>>> you > > > > > > > > > >>>>>>>>>>>>>>> do > > > > > > > > > >>>>>>>>>>>>>>>>> refactorings on your own. > > > > > > > > > >>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>> On Tue, Apr 24, 2018 at 10:52 PM, Andrey > > > > > > > > > >>> Kuznetsov > > > > > > > > > >>>>> < > > > > > > > > > >>>>>>>>>>>>> stku...@gmail.com > > > > > > > > > >>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>> wrote: > > > > > > > > > >>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> +1. > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> Once again, I beg for "small refactoring > > > > > > > > > >>>>> permission" > > > > > > > > > >>>>>>> in > > > > > > > > > >>>>>>>> a > > > > > > > > > >>>>>>>>>>>>> checklist. > > > > > > > > > >>>>>>>>>>>>>>>> As of > > > > > > > > > >>>>>>>>>>>>>>>>>> today, separate tickets for small > > > > > > > > > >> refactorings > > > > > > > > > >>>>> has > > > > > > > > > >>>>>>>> lowest > > > > > > > > > >>>>>>>>>>>>> priority, > > > > > > > > > >>>>>>>>>>>>>>>> since > > > > > > > > > >>>>>>>>>>>>>>>>>> they neither fix any flaw nor add new > > > > > > > > > >>>>> functionality. > > > > > > > > > >>>>>>>> Also, > > > > > > > > > >>>>>>>>>> the > > > > > > > > > >>>>>>>>>>>>>>>> attempts to > > > > > > > > > >>>>>>>>>>>>>>>>>> make issue-related code safer / cleaner > / > > > > > > > > > >> more > > > > > > > > > >>>>>>> readable > > > > > > > > > >>>>>>>> in > > > > > > > > > >>>>>>>>>>>> "real" > > > > > > > > > >>>>>>>>>>>>>>> pull > > > > > > > > > >>>>>>>>>>>>>>>>>> requests are typically rejected, since > > they > > > > > > > > > >>>>>> contradict > > > > > > > > > >>>>>>>> our > > > > > > > > > >>>>>>>>>>>> current > > > > > > > > > >>>>>>>>>>>>>>>>>> guidelines. > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> I understand this will require a bit > more > > > > > > > > > >>> effort > > > > > > > > > >>>>>> from > > > > > > > > > >>>>>>>>>>>>>>>> committer/maintainer, > > > > > > > > > >>>>>>>>>>>>>>>>>> but otherwise we will get constantly > > > > > > > > > >> degrading > > > > > > > > > >>>>> code > > > > > > > > > >>>>>>>>> quality. > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> 2018-04-24 18:52 GMT+03:00 Eduard > > Shangareev > > > > > > > > > >> < > > > > > > > > > >>>>>>>>>>>>>>>> eduard.shangar...@gmail.com > > > > > > > > > >>>>>>>>>>>>>>>>>>> : > > > > > > > > > >>>>>>>>>>>>>>>>>>> Vladimir, > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> I am not talking about > > > > > > > > > >> massive/sophisticated > > > > > > > > > >>>>>>>>> refactoring. > > > > > > > > > >>>>>>>>>>> But > > > > > > > > > >>>>>>>>>>>> I > > > > > > > > > >>>>>>>>>>>>>>>> believe > > > > > > > > > >>>>>>>>>>>>>>>>>>> that ask to extract some methods should > > be > > > > > > > > > >> OK > > > > > > > > > >>>>> to > > > > > > > > > >>>>>> do > > > > > > > > > >>>>>>>>>> without > > > > > > > > > >>>>>>>>>>> an > > > > > > > > > >>>>>>>>>>>>>>> extra > > > > > > > > > >>>>>>>>>>>>>>>>>>> ticket. > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> A checklist shouldn't be necessarily a > > set > > > > > > > > > >> of > > > > > > > > > >>>>>>> certain > > > > > > > > > >>>>>>>>>> rules > > > > > > > > > >>>>>>>>>>>> but > > > > > > > > > >>>>>>>>>>>>>>> also > > > > > > > > > >>>>>>>>>>>>>>>> it > > > > > > > > > >>>>>>>>>>>>>>>>>>> could include suggestion and reminders. > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> On Tue, Apr 24, 2018 at 6:39 PM, > Vladimir > > > > > > > > > >>>>> Ozerov < > > > > > > > > > >>>>>>>>>>>>>>>> voze...@gridgain.com> > > > > > > > > > >>>>>>>>>>>>>>>>>>> wrote: > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> Ed, > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> Refactoring is a separate task. If you > > > > > > > > > >>> would > > > > > > > > > >>>>>> like > > > > > > > > > >>>>>>> to > > > > > > > > > >>>>>>>>>>> rework > > > > > > > > > >>>>>>>>>>>>>>>> exchange > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> future > > > > > > > > > >>>>>>>>>>>>>>>>>>>> - please do this in a ticket "Refactor > > > > > > > > > >>>>> exchange > > > > > > > > > >>>>>>>> task", > > > > > > > > > >>>>>>>>>>>> nobody > > > > > > > > > >>>>>>>>>>>>>>> would > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> against > > > > > > > > > >>>>>>>>>>>>>>>>>>>> this. This is just a matter of > creating > > > > > > > > > >>>>> separate > > > > > > > > > >>>>>>>>> ticket > > > > > > > > > >>>>>>>>>>> and > > > > > > > > > >>>>>>>>>>>>>>>> separate > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> PR. > > > > > > > > > >>>>>>>>>>>>>>>>>>> If > > > > > > > > > >>>>>>>>>>>>>>>>>>>> one have a time for refactoring, it > > > > > > > > > >> should > > > > > > > > > >>>>> not > > > > > > > > > >>>>>> be > > > > > > > > > >>>>>>> a > > > > > > > > > >>>>>>>>>>> problem > > > > > > > > > >>>>>>>>>>>>> for > > > > > > > > > >>>>>>>>>>>>>>>> him to > > > > > > > > > >>>>>>>>>>>>>>>>>>>> spend several minutes on JIRA and > > GitHub. > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> As far as documentation - what you > > > > > > > > > >> describe > > > > > > > > > >>>>> is > > > > > > > > > >>>>>>>> normal > > > > > > > > > >>>>>>>>>>> review > > > > > > > > > >>>>>>>>>>>>>>>> process, > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> when > > > > > > > > > >>>>>>>>>>>>>>>>>>>> reviewer might want to ask contributor > > to > > > > > > > > > >>> fix > > > > > > > > > >>>>>>>>> something. > > > > > > > > > >>>>>>>>>>>>>>> Checklist > > > > > > > > > >>>>>>>>>>>>>>>> is a > > > > > > > > > >>>>>>>>>>>>>>>>>>>> different thing - this is a set of > rules > > > > > > > > > >>>>> which > > > > > > > > > >>>>>>> must > > > > > > > > > >>>>>>>> be > > > > > > > > > >>>>>>>>>>>>> followed > > > > > > > > > >>>>>>>>>>>>>>> by > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> anyone. > > > > > > > > > >>>>>>>>>>>>>>>>>>>> I do not understand how you can define > > > > > > > > > >>>>>>> documentation > > > > > > > > > >>>>>>>>> in > > > > > > > > > >>>>>>>>>>> this > > > > > > > > > >>>>>>>>>>>>>>>> checklist. > > > > > > > > > >>>>>>>>>>>>>>>>>>>> Same problem with logging - what is > > > > > > > > > >>> "enough"? > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> On Tue, Apr 24, 2018 at 4:51 PM, > Eduard > > > > > > > > > >>>>>>> Shangareev < > > > > > > > > > >>>>>>>>>>>>>>>>>>>> eduard.shangar...@gmail.com> wrote: > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> Igniters, > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> I don't understand why you are so > > > > > > > > > >> against > > > > > > > > > >>>>>>>>> refactoring. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> Code already smells like hell. > Methods > > > > > > > > > >>> 200+ > > > > > > > > > >>>>>> line > > > > > > > > > >>>>>>>> is > > > > > > > > > >>>>>>>>>>>> normal. > > > > > > > > > >>>>>>>>>>>>>>>> Exchange > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> future > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> is asking to be separated on several > > > > > > > > > >> one. > > > > > > > > > >>>>>>>>> Transaction > > > > > > > > > >>>>>>>>>>> code > > > > > > > > > >>>>>>>>>>>>>>> could > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> understand > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> few people. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> If we separate refactoring from > > > > > > > > > >>>>> development it > > > > > > > > > >>>>>>>> would > > > > > > > > > >>>>>>>>>>> mean > > > > > > > > > >>>>>>>>>>>>> that > > > > > > > > > >>>>>>>>>>>>>>>> no one > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> will > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> do it. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> 2) Documentation. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> Everything which was asked by > reviewers > > > > > > > > > >>> to > > > > > > > > > >>>>>>> clarify > > > > > > > > > >>>>>>>>>> idea > > > > > > > > > >>>>>>>>>>>>>>> should be > > > > > > > > > >>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>> reflected > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> in the code. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> 3) Logging. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> Logging should be enough to > > > > > > > > > >> troubleshoot > > > > > > > > > >>>>> the > > > > > > > > > >>>>>>>> problem > > > > > > > > > >>>>>>>>>> if > > > > > > > > > >>>>>>>>>>>>>>> someone > > > > > > > > > >>>>>>>>>>>>>>>> comes > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> to > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> user-list with an issue in the code. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> On Fri, Apr 20, 2018 at 7:06 PM, > Dmitry > > > > > > > > > >>>>>> Pavlov < > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> dpavlov....@gmail.com> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> wrote: > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> Hi Igniters, > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> +1 to idea of checklist. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> +1 to refactoring and documenting > > > > > > > > > >> code > > > > > > > > > >>>>>> related > > > > > > > > > >>>>>>>> to > > > > > > > > > >>>>>>>>>>> ticket > > > > > > > > > >>>>>>>>>>>>> in > > > > > > > > > >>>>>>>>>>>>>>>> +/-20 > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> LOC > > > > > > > > > >>>>>>>>>>>>>>>>>>>> at > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> least. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> If we start to do it as part of our > > > > > > > > > >>>>> regular > > > > > > > > > >>>>>>>>>>>> contribution, > > > > > > > > > >>>>>>>>>>>>>>> code > > > > > > > > > >>>>>>>>>>>>>>>> will > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> be > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> better, it would became common > > > > > > > > > >> practice > > > > > > > > > >>>>> and > > > > > > > > > >>>>>>> part > > > > > > > > > >>>>>>>>> of > > > > > > > > > >>>>>>>>>>>> Apache > > > > > > > > > >>>>>>>>>>>>>>>> Ignite > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> development culure. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> If we will hope we will have free > > > > > > > > > >> time > > > > > > > > > >>> to > > > > > > > > > >>>>>>> submit > > > > > > > > > >>>>>>>>>>>> separate > > > > > > > > > >>>>>>>>>>>>>>> patch > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> someday > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> and > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> have patience to complete > > > > > > > > > >>>>> patch-submission > > > > > > > > > >>>>>>>>> process, > > > > > > > > > >>>>>>>>>>> code > > > > > > > > > >>>>>>>>>>>>>>> will > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> remain > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> undocumented and poor-readable. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> Sincerely, > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> Dmitriy Pavlov > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> пт, 20 апр. 2018 г. в 18:56, > > > > > > > > > >> Александр > > > > > > > > > >>>>>>>> Меньшиков < > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> sharple...@gmail.com > > > > > > > > > >>>>>>>>>>>>>>>>>>>>> : > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> 4) Metrics. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> partially +1 > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> It makes sense to have some minimal > > > > > > > > > >>>>> code > > > > > > > > > >>>>>>>>> coverage > > > > > > > > > >>>>>>>>>>> for > > > > > > > > > >>>>>>>>>>>>> new > > > > > > > > > >>>>>>>>>>>>>>>> code in > > > > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>> PR. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>> IMHO. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> Also, we can limit the cyclomatic > > > > > > > > > >>>>>> complexity > > > > > > > > > >>>>>>>> of > > > > > > > > > >>>>>>>>>> the > > > > > > > > > >>>>>>>>>>>> new > > > > > > > > > >>>>>>>>>>>>>>> code > > > > > > > > > >>>>>>>>>>>>>>>> in > > > > > > > > > >>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>> PR > > > > > > > > > >>>>>>>>>>>>>>>>>>>> too. > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> 6) Refactoring > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> -1 > > > > > > > > > >>>>>>>>>>>>>>>>>>>>>>> > > > > > > > > > >>>>>>>>>>>>>>>>>>>>
