Dmitrii, See my comments at issue. Next time, please leave review request inside the issue.
On Wed, Jan 9, 2019 at 6:39 PM Dmitrii Ryabov <somefire...@gmail.com> wrote: > Hi, > > Anton, can you review ticket for examples [1]? > > [1] https://issues.apache.org/jira/browse/IGNITE-6826 > > пн, 24 дек. 2018 г., 15:23 Anton Vinogradov a...@apache.org: > > > Folks, > > > > The important thing here is that 99% of "final static" ip-finders were > > removed. (~800 pcs.) > > Non-static, mostly, kept as is since removal may or cause a test failure. > > (~130 pcs.) > > > > In case someone interested in the continuation of cleanup, please feel > free > > to create an issue and provide PR, I'm ready to review it. > > > > On Mon, Dec 24, 2018 at 3:04 PM Vyacheslav Daradur <daradu...@gmail.com> > > wrote: > > > > > The second step [2] "removing related boilerplate in tests" - has been > > > done. It is expected that a bit memory will be released at tests TC > > > agents, which could not be cleaned by GC because of static final > > > fields. > > > > > > Guys, please, do not generate a new one ) > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-10715 > > > > > > On Tue, Dec 18, 2018 at 6:29 PM Anton Vinogradov <a...@apache.org> > wrote: > > > > > > > > Folks, > > > > > > > > Now I see 5-10% speedup at all TC suites right after the merge. > > > > Great fix! > > > > > > > > > > > > On Mon, Dec 17, 2018 at 2:39 PM Vyacheslav Daradur < > > daradu...@gmail.com> > > > > wrote: > > > > > > > > > Andrey Mashenkov, at first sight, I have not seen any problems with > > > > > .NET platform. > > > > > > > > > > I believe we need carefully configure ports in platform's examples, > > > > > additional actions should not be required. > > > > > > > > > > On Mon, Dec 17, 2018 at 2:35 PM Vyacheslav Daradur < > > > daradu...@gmail.com> > > > > > wrote: > > > > > > > > > > > > The task [1] is done. > > > > > > > > > > > > 'TcpDiscoveryVmIpFinder' is default IP finder in tests now. > > > > > > > > > > > > The IP finder is initialized on per tests class level to avoid > > hidden > > > > > > affecting among tests, it means the test methods in the common > test > > > > > > class will use the same IP finder. > > > > > > > > > > > > You don't need to set up 'TcpDiscoveryVmIpFinder' in your tests > > > > > > explicitly anymore, also task [2] has been filled to remove > related > > > > > > boilerplate if nobody minds. > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-10555 > > > > > > [2] https://issues.apache.org/jira/browse/IGNITE-10715 > > > > > > > > > > > > > > > > > > On Wed, Dec 5, 2018 at 7:17 PM Dmitriy Pavlov < > dpav...@apache.org> > > > > > wrote: > > > > > > > > > > > > > > ++1 > > > > > > > > > > > > > > ср, 5 дек. 2018 г. в 18:38, Denis Mekhanikov < > > > dmekhani...@gmail.com>: > > > > > > > > > > > > > > > Andrey, > > > > > > > > > > > > > > > > Multi-JVM tests may also use a static IP finder, but it > should > > > use > > > > > some > > > > > > > > specific port range instead of being shared. > > > > > > > > Something like 127.0.0.1:48500..48509 would do. > > > > > > > > > > > > > > > > Denis > > > > > > > > > > > > > > > > ср, 5 дек. 2018 г. в 18:34, Vyacheslav Daradur < > > > daradu...@gmail.com > > > > > >: > > > > > > > > > > > > > > > > > I filled a task [1]. > > > > > > > > > > > > > > > > > > >> Slava, do you think Platforms tests can be fixed as well > > or > > > one > > > > > more > > > > > > > > > ticket > > > > > > > > > should be created? > > > > > > > > > > > > > > > > > > I'll try to fix them within one ticket, it should be > > > investigated > > > > > a bit > > > > > > > > > deeper. > > > > > > > > > > > > > > > > > > I'll inform about the task's progress in this thread later. > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-10555 > > > > > > > > > On Wed, Dec 5, 2018 at 6:28 PM Andrey Mashenkov > > > > > > > > > <andrey.mashen...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > Slava, > > > > > > > > > > +1 for your proposal. > > > > > > > > > > Is there any ticket for this? > > > > > > > > > > > > > > > > > > > > Denis, > > > > > > > > > > I've just read in nabble thread you suggest to allow > > > multicast > > > > > finder > > > > > > > > for > > > > > > > > > > multiJVM tests > > > > > > > > > > and I'd think we shouldn't use multicast in test at all > > > (excepts > > > > > > > > > multicast > > > > > > > > > > Ip finder self tests of course), > > > > > > > > > > but e.g. add an assertion to force user to create > ipfinder > > > > > properly. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Also, we have a ticket for similar issue in 'examples' > > > module. > > > > > > > > > > Seems, there are some issues with Platforms module > > > integration. > > > > > > > > > > Slava, do you think Platforms tests can be fixed as well > or > > > one > > > > > more > > > > > > > > > ticket > > > > > > > > > > should be created? > > > > > > > > > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-6826 > > > > > > > > > > > > > > > > > > > > On Wed, Dec 5, 2018 at 5:55 PM Denis Mekhanikov < > > > > > dmekhani...@gmail.com > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Slava, > > > > > > > > > > > > > > > > > > > > > > These are exactly my thoughts, so I fully support you > > here. > > > > > > > > > > > I already wrote about it: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > http://apache-ignite-developers.2346864.n4.nabble.com/IP-finder-in-tests-td33322.html > > > > > > > > > > > But I kind of abandoned this activity. Feel free to > take > > > over > > > > > it. > > > > > > > > > > > > > > > > > > > > > > Denis > > > > > > > > > > > > > > > > > > > > > > ср, 5 дек. 2018 г. в 17:22, Vladimir Ozerov < > > > > > voze...@gridgain.com>: > > > > > > > > > > > > > > > > > > > > > > > Huge +1 > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Dec 5, 2018 at 5:09 PM Vyacheslav Daradur < > > > > > > > > > daradu...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Igniters, > > > > > > > > > > > > > > > > > > > > > > > > > > I've found that the project's test framework uses > > > > > > > > > > > > > 'TcpDiscoveryMulticastIpFinder' as default IP > finder > > > for > > > > > tests > > > > > > > > and > > > > > > > > > > > > > there are a lot of tests written by Ignite's > experts > > > that > > > > > > > > override > > > > > > > > > it > > > > > > > > > > > > > to 'TcpDiscoveryVmIpFinder'. > > > > > > > > > > > > > > > > > > > > > > > > > > Most of our tests starting Ignite nodes in the same > > > JVM, > > > > > that > > > > > > > > > allows > > > > > > > > > > > > > us using shared 'TcpDiscoveryVmIpFinder'. > > > > > > > > > > > > > > > > > > > > > > > > > > I think that using of > 'TcpDiscoveryMulticastIpFinder' > > > may > > > > > be > > > > > > > > useful > > > > > > > > > > > > > only in platforms tests, BTW multi-JVM tests use > the > > > tuned > > > > > > > > > > > > > 'TcpDiscoveryVmIpFinder'. > > > > > > > > > > > > > > > > > > > > > > > > > > I see the following main advantages of using > > > > > > > > > 'TcpDiscoveryVmIpFinder': > > > > > > > > > > > > > * reducing possible conflicts in the development > > > > > environment, > > > > > > > > when > > > > > > > > > > > > > nodes from different clusters may find each other; > > > > > > > > > > > > > * speedup of nodes initial discovery, especially on > > > > > Windows; > > > > > > > > > > > > > * avoiding of overwriting 'getConfiguration' and > > > copypasta > > > > > only > > > > > > > > to > > > > > > > > > set > > > > > > > > > > > > > up static IP finder in tests; > > > > > > > > > > > > > > > > > > > > > > > > > > So, I'd suggest changing the default IP finder in > > > tests to > > > > > > > > > > > > > 'TcpDiscoveryVmIpFinder' as the first step and > remove > > > > > related > > > > > > > > > > > > > boilerplate as the second step. > > > > > > > > > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Best regards, > > > > > > > > > > Andrey V. Mashenkov > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > -- > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > -- > > > Best Regards, Vyacheslav D. > > > > > >
